From 72393343a2b5065c01f74e21329998f436e726ad Mon Sep 17 00:00:00 2001 From: Andrew Kushnir Date: Tue, 25 Jul 2023 17:01:21 -0700 Subject: [PATCH] refactor(compiler): drop regular imports when symbols can be defer-loaded This commit updates the logic to drop regular imports when all symbols that it brings can be defer-loaded. The change ensures that there is no mix of regular and dynamic imports present in a source file. --- .../src/ngtsc/annotations/common/src/util.ts | 4 +- .../annotations/component/src/handler.ts | 3 +- .../src/ngtsc/transform/src/api.ts | 1 + .../src/ngtsc/transform/src/transform.ts | 26 ++- .../src/ngtsc/util/src/visitor.ts | 4 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 175 +++++++++++++++++- 6 files changed, 203 insertions(+), 10 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/common/src/util.ts b/packages/compiler-cli/src/ngtsc/annotations/common/src/util.ts index 484ff27715924..e5cd012542956 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/common/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/common/src/util.ts @@ -331,7 +331,8 @@ export function createSourceSpan(node: ts.Node): ParseSourceSpan { */ export function compileResults( fac: CompileResult, def: R3CompiledExpression, metadataStmt: Statement|null, propName: string, - additionalFields: CompileResult[]|null): CompileResult[] { + additionalFields: CompileResult[]|null, + deferrableImports?: Set): CompileResult[] { const statements = def.statements; if (metadataStmt !== null) { statements.push(metadataStmt); @@ -344,6 +345,7 @@ export function compileResults( initializer: def.expression, statements: def.statements, type: def.type, + deferrableImports, }, ]; diff --git a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts index 6bc6a64f257ef..849315b083fa2 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -1006,7 +1006,8 @@ export class ComponentDecoratorHandler implements const classMetadata = analysis.classMetadata !== null ? compileClassMetadata(analysis.classMetadata).toStmt() : null; - return compileResults(fac, def, classMetadata, 'ɵcmp', inputTransformFields); + const deferrableImports = this.deferredSymbolTracker.getDeferrableImportDecls(); + return compileResults(fac, def, classMetadata, 'ɵcmp', inputTransformFields, deferrableImports); } compilePartial( diff --git a/packages/compiler-cli/src/ngtsc/transform/src/api.ts b/packages/compiler-cli/src/ngtsc/transform/src/api.ts index 1a8f73ac79e12..18be10e1705bd 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/api.ts @@ -248,6 +248,7 @@ export interface CompileResult { initializer: Expression|null; statements: Statement[]; type: Type; + deferrableImports?: Set; } export interface ResolveResult { diff --git a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts index 90193f1f8ebf4..66ff1cb133003 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/transform.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/transform.ts @@ -56,6 +56,7 @@ export function ivyTransformFactory( */ class IvyCompilationVisitor extends Visitor { public classCompilationMap = new Map(); + public deferrableImports = new Set(); constructor(private compilation: TraitCompiler, private constantPool: ConstantPool) { super(); @@ -68,6 +69,16 @@ class IvyCompilationVisitor extends Visitor { const result = this.compilation.compile(node, this.constantPool); if (result !== null) { this.classCompilationMap.set(node, result); + + // Collect all deferrable imports declarations into a single set, + // so that we can pass it to the transform visitor that will drop + // corresponding regular import declarations. + for (const classResult of result) { + if (classResult.deferrableImports && classResult.deferrableImports.size > 0) { + classResult.deferrableImports.forEach( + importDecl => this.deferrableImports.add(importDecl)); + } + } } return {node}; } @@ -83,7 +94,8 @@ class IvyTransformationVisitor extends Visitor { private classCompilationMap: Map, private reflector: ReflectionHost, private importManager: ImportManager, private recordWrappedNodeExpr: RecordWrappedNodeFn, - private isClosureCompilerEnabled: boolean, private isCore: boolean) { + private isClosureCompilerEnabled: boolean, private isCore: boolean, + private deferrableImports: Set) { super(); } @@ -153,6 +165,16 @@ class IvyTransformationVisitor extends Visitor { return {node, after: statements}; } + override visitOtherNode(node: T): T { + if (ts.isImportDeclaration(node) && this.deferrableImports.has(node)) { + // Return `null` as an indication that this node should not be present + // in the final AST. Symbols from this import would be imported via + // dynamic imports. + return null!; + } + return node; + } + /** * Return all decorators on a `Declaration` which are from @angular/core, or an empty set if none * are. @@ -281,7 +303,7 @@ function transformIvySourceFile( // results obtained at Step 1. const transformationVisitor = new IvyTransformationVisitor( compilation, compilationVisitor.classCompilationMap, reflector, importManager, - recordWrappedNode, isClosureCompilerEnabled, isCore); + recordWrappedNode, isClosureCompilerEnabled, isCore, compilationVisitor.deferrableImports); let sf = visit(file, transformationVisitor, context); // Generate the constant statements first, as they may involve adding additional imports diff --git a/packages/compiler-cli/src/ngtsc/util/src/visitor.ts b/packages/compiler-cli/src/ngtsc/util/src/visitor.ts index 503d3924bd651..e41ddac539564 100644 --- a/packages/compiler-cli/src/ngtsc/util/src/visitor.ts +++ b/packages/compiler-cli/src/ngtsc/util/src/visitor.ts @@ -78,7 +78,7 @@ export abstract class Visitor { // is completed. let visitedNode: T|null = null; - node = ts.visitEachChild(node, child => this._visit(child, context), context) as T; + node = ts.visitEachChild(node, child => child && this._visit(child, context), context) as T; if (ts.isClassDeclaration(node)) { visitedNode = @@ -90,7 +90,7 @@ export abstract class Visitor { // If the visited node has a `statements` array then process them, maybe replacing the visited // node and adding additional statements. - if (ts.isBlock(visitedNode) || ts.isSourceFile(visitedNode)) { + if (visitedNode && (ts.isBlock(visitedNode) || ts.isSourceFile(visitedNode))) { visitedNode = this._maybeProcessStatements(visitedNode); } diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index abe03b0d0dfc0..676c0520f7acf 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -8873,10 +8873,10 @@ function allTests(os: string) { }); describe('deferred blocks', () => { - it('should not error for deferred blocks', () => { + it('should handle deferred blocks', () => { env.tsconfig({_enabledBlockTypes: ['defer']}); env.write('cmp-a.ts', ` - import {Component} from '@angular/core'; + import { Component } from '@angular/core'; @Component({ standalone: true, @@ -8887,8 +8887,8 @@ function allTests(os: string) { `); env.write('/test.ts', ` - import {Component} from '@angular/core'; - import {CmpA} from './cmp-a'; + import { Component } from '@angular/core'; + import { CmpA } from './cmp-a'; @Component({ selector: 'local-dep', @@ -8914,10 +8914,177 @@ function allTests(os: string) { env.driveMain(); const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('ɵɵdefer(0, TestCmp_Defer_0_DepsFn)'); expect(jsContents) .toContain( 'function TestCmp_Defer_0_DepsFn() { return [import("./cmp-a").then(function (m) { return m.CmpA; }), LocalDep]; }'); + + // The `CmpA` symbol wasn't referenced elsewhere, so it can be defer-loaded + // via dynamic imports and an original import can be removed. + expect(jsContents).not.toContain('import { CmpA }'); + }); + + describe('imports', () => { + it('should retain regular imports when symbol is eagerly referenced', () => { + env.tsconfig({_enabledBlockTypes: ['defer']}); + env.write('cmp-a.ts', ` + import { Component } from '@angular/core'; + + @Component({ + standalone: true, + selector: 'cmp-a', + template: 'CmpA!' + }) + export class CmpA {} + `); + + env.write('/test.ts', ` + import { Component } from '@angular/core'; + import { CmpA } from './cmp-a'; + + @Component({ + selector: 'test-cmp', + standalone: true, + imports: [CmpA], + template: \` + {#defer} + + {/defer} + \`, + }) + export class TestCmp { + constructor() { + // This line retains the regular import of CmpA, + // since it's eagerly referenced in the code. + console.log(CmpA); + } + } + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + + expect(jsContents).toContain('ɵɵdefer(0, TestCmp_Defer_0_DepsFn)'); + + // The dependency function doesn't have a dynamic import, because `CmpA` + // was eagerly referenced in component's code, thus regular import can not be removed. + expect(jsContents).toContain('function TestCmp_Defer_0_DepsFn() { return [CmpA]; }'); + expect(jsContents).toContain('import { CmpA }'); + }); + + it('should retain regular imports when one of the symbols is eagerly referenced', () => { + env.tsconfig({_enabledBlockTypes: ['defer']}); + env.write('cmp-a.ts', ` + import { Component } from '@angular/core'; + + @Component({ + standalone: true, + selector: 'cmp-a', + template: 'CmpA!' + }) + export class CmpA {} + + @Component({ + standalone: true, + selector: 'cmp-b', + template: 'CmpB!' + }) + export class CmpB {} + `); + + env.write('/test.ts', ` + import { Component } from '@angular/core'; + import { CmpA, CmpB } from './cmp-a'; + + @Component({ + selector: 'test-cmp', + standalone: true, + imports: [CmpA, CmpB], + template: \` + {#defer} + + + {/defer} + \`, + }) + export class TestCmp { + constructor() { + // This line retains the regular import of CmpA, + // since it's eagerly referenced in the code. + console.log(CmpA); + } + } + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + + expect(jsContents).toContain('ɵɵdefer(0, TestCmp_Defer_0_DepsFn)'); + + // The dependency function doesn't have a dynamic import, because `CmpA` + // was eagerly referenced in component's code, thus regular import can not be removed. + // This also affects `CmpB`, since it was extracted from the same import. + expect(jsContents) + .toContain('function TestCmp_Defer_0_DepsFn() { return [CmpA, CmpB]; }'); + expect(jsContents).toContain('import { CmpA, CmpB }'); + }); + + it('should drop regular imports when none of the symbols are eagerly referenced', () => { + env.tsconfig({_enabledBlockTypes: ['defer']}); + env.write('cmp-a.ts', ` + import { Component } from '@angular/core'; + + @Component({ + standalone: true, + selector: 'cmp-a', + template: 'CmpA!' + }) + export class CmpA {} + + @Component({ + standalone: true, + selector: 'cmp-b', + template: 'CmpB!' + }) + export class CmpB {} + `); + + env.write('/test.ts', ` + import { Component } from '@angular/core'; + import { CmpA, CmpB } from './cmp-a'; + + @Component({ + selector: 'test-cmp', + standalone: true, + imports: [CmpA, CmpB], + template: \` + {#defer} + + + {/defer} + \`, + }) + export class TestCmp {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + + expect(jsContents).toContain('ɵɵdefer(0, TestCmp_Defer_0_DepsFn)'); + + // Both `CmpA` and `CmpB` were used inside the `{#defer}` and were not + // referenced elsewhere, so we generate dynamic imports and drop a regular one. + expect(jsContents) + .toContain( + 'function TestCmp_Defer_0_DepsFn() { return [' + + 'import("./cmp-a").then(function (m) { return m.CmpA; }), ' + + 'import("./cmp-a").then(function (m) { return m.CmpB; })]; }'); + expect(jsContents).not.toContain('import { CmpA, CmpB }'); + }); }); }); });