From 0fbf375d0742e868a46392089f1af164ac670b9d Mon Sep 17 00:00:00 2001 From: Payam Valadkhan Date: Thu, 25 Apr 2024 16:57:08 -0400 Subject: [PATCH] refactor(compiler-cli): optimize extra import generation in local compilation mode Currently we add global extra imports to all the files in the compilation unit. However not all the files need extra imports. For example non-Angular files definitely do not need such extra imports, and in some cases these extra imports causes problems as the file is meant to be run the Node but it has Angular dependencies which are not compatible with Node. This change tries to limit extra import generation to a subset of files. Wit hthis change we create extra imports only for the files that contain at least one component whose NgModule is in a different file. This is because all other files do not need extra imports since they are either not Angular files or they already have all the imports that the components need. --- .../annotations/component/src/handler.ts | 6 + ...local_compilation_extra_imports_tracker.ts | 19 ++ .../test/ngtsc/local_compilation_spec.ts | 209 +++++++++++++++--- 3 files changed, 199 insertions(+), 35 deletions(-) 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 6179ef9b6cd6e9..aecc8b4db67c5a 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts @@ -774,6 +774,12 @@ export class ComponentDecoratorHandler implements // Dependencies from the `@Component.deferredImports` field. const explicitlyDeferredDependencies = getExplicitlyDeferredDeps(scope); + // Mark the component is an NgModule-based component with its NgModule in a different file + // then mark this file for extra import generation + if (isModuleScope && context.fileName !== getSourceFile(scope.ngModule).fileName) { + this.localCompilationExtraImportsTracker?.markFileForExtraImportGeneration(context); + } + // Make sure that `@Component.imports` and `@Component.deferredImports` do not have // the same dependencies. if (metadata.isStandalone && analysis.rawDeferredImports !== null && diff --git a/packages/compiler-cli/src/ngtsc/imports/src/local_compilation_extra_imports_tracker.ts b/packages/compiler-cli/src/ngtsc/imports/src/local_compilation_extra_imports_tracker.ts index a895313f80737b..b58dd479a440d4 100644 --- a/packages/compiler-cli/src/ngtsc/imports/src/local_compilation_extra_imports_tracker.ts +++ b/packages/compiler-cli/src/ngtsc/imports/src/local_compilation_extra_imports_tracker.ts @@ -37,8 +37,23 @@ export class LocalCompilationExtraImportsTracker { private readonly localImportsMap = new Map>(); private readonly globalImportsSet = new Set(); + /** Names of the files marked for extra import generation. */ + private readonly markedFilesSet = new Set(); + constructor(private readonly typeChecker: ts.TypeChecker) {} + /** + * Marks the source file for extra imports generation. + * + * The extra imports are generated only for the files marked through this method. In other words, + * the method {@link getImportsForFile} returns empty if the file is not marked. This allows the + * consumers of this tool to avoid generating extra imports for unrelated files (e.g., non-Angular + * files) + */ + markFileForExtraImportGeneration(sf: ts.SourceFile) { + this.markedFilesSet.add(sf.fileName); + } + /** * Adds an extra import to be added to the generated file of a specific source file. */ @@ -91,6 +106,10 @@ export class LocalCompilationExtraImportsTracker { * Returns the list of all module names that the given file should include as its extra imports. */ getImportsForFile(sf: ts.SourceFile): string[] { + if (!this.markedFilesSet.has(sf.fileName)) { + return []; + } + return [ ...this.globalImportsSet, ...(this.localImportsMap.get(sf.fileName) ?? []), diff --git a/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts b/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts index 2e333b165b4d10..914d215f4548bb 100644 --- a/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts +++ b/packages/compiler-cli/test/ngtsc/local_compilation_spec.ts @@ -68,6 +68,91 @@ runInEachFileSystem(() => { }); it('should only include NgModule external import as global import', () => { + env.write('comp1.ts', ` + import {Component} from '@angular/core'; + + @Component({template:''}) + export class Comp1 { + } + `); + env.write('module1.ts', ` + import {NgModule} from '@angular/core'; + + import {Comp1} from 'comp1'; + + @NgModule({declarations:[Comp1]}) + export class Module1 { + } + `); + env.write('a.ts', ` + import {NgModule} from '@angular/core'; + import {SomeExternalStuff} from '/some_external_file'; + import {SomeExternalStuff2} from '/some_external_file2'; + + import {BModule} from 'b'; + + @NgModule({imports: [SomeExternalStuff, BModule]}) + export class AModule { + } + `); + env.write('b.ts', ` + import {NgModule} from '@angular/core'; + + @NgModule({}) + export class BModule { + } + `); + + env.driveMain(); + const Comp1Contents = env.getContents('comp1.js'); + + expect(Comp1Contents) + .withContext('NgModule external imports should be included in the global imports') + .toContain('import "/some_external_file"'); + expect(Comp1Contents) + .withContext( + 'External import which is not an NgModule import should not be included in the global import') + .not.toContain('import "/some_external_file2"'); + expect(Comp1Contents) + .withContext('NgModule internal import should not be included in the global import') + .not.toContain('import "b"'); + }); + + it('should include global imports only in the eligible files', () => { + env.write('module_and_comp.ts', ` + import {NgModule, Component} from '@angular/core'; + + @Component({template:'', standalone: true}) + export class Comp3 { + } + + @NgModule({declarations:[Comp3]}) + export class Module3 { + } + `); + env.write('standalone_comp.ts', ` + import {Component} from '@angular/core'; + + @Component({template:'', standalone: true}) + export class Comp2 { + } + `); + env.write('comp1.ts', ` + import {Component} from '@angular/core'; + + @Component({template:''}) + export class Comp1 { + } + `); + env.write('module1.ts', ` + import {NgModule} from '@angular/core'; + + import {Comp1} from 'comp1'; + + @NgModule({declarations:[Comp1]}) + export class Module1 { + } + `); env.write('a.ts', ` import {NgModule} from '@angular/core'; import {SomeExternalStuff} from '/some_external_file'; @@ -91,27 +176,44 @@ runInEachFileSystem(() => { `); env.driveMain(); - const aContents = env.getContents('a.js'); - const bContents = env.getContents('b.js'); - const cContents = env.getContents('c.js'); - - // NgModule external import as global import - expect(aContents).toContain('import "/some_external_file"'); - expect(bContents).toContain('import "/some_external_file"'); - expect(cContents).toContain('import "/some_external_file"'); - - // External import which is not an NgModule import should not be global import - expect(aContents).not.toContain('import "/some_external_file2"'); - expect(bContents).not.toContain('import "/some_external_file2"'); - expect(cContents).not.toContain('import "/some_external_file2"'); - - // NgModule internal import should not be global import - expect(aContents).not.toContain('import "b"'); - expect(bContents).not.toContain('import "b"'); - expect(cContents).not.toContain('import "b"'); + + expect(env.getContents('comp1.js')) + .withContext( + 'Global imports should be generated when a component has its NgModule in a different file') + .toContain('import "/some_external_file"'); + expect(env.getContents('standalone_comp.js')) + .withContext( + 'Global imports should not be generated when all components are standalone') + .not.toContain('import "/some_external_file"'); + expect(env.getContents('module_and_comp.js')) + .withContext( + 'Global imports should not be generated when components and their NgModules are in the same file') + .not.toContain('import "/some_external_file"'); + expect(env.getContents('a.js')) + .withContext('Global imports should not be generated when the file has no component') + .not.toContain('import "/some_external_file"'); + expect(env.getContents('c.js')) + .withContext('Global imports should not be generated for non-Angular files') + .not.toContain('import "/some_external_file"'); }); it('should include NgModule namespace external import as global import', () => { + env.write('comp1.ts', ` + import {Component} from '@angular/core'; + + @Component({template:''}) + export class Comp1 { + } + `); + env.write('module1.ts', ` + import {NgModule} from '@angular/core'; + + import {Comp1} from 'comp1'; + + @NgModule({declarations:[Comp1]}) + export class Module1 { + } + `); env.write('a.ts', ` import {NgModule} from '@angular/core'; import * as n from '/some_external_file'; @@ -128,12 +230,27 @@ runInEachFileSystem(() => { env.driveMain(); - expect(env.getContents('a.js')).toContain('import "/some_external_file"'); - expect(env.getContents('test.js')).toContain('import "/some_external_file"'); + expect(env.getContents('comp1.js')).toContain('import "/some_external_file"'); }); it('should include nested NgModule external import as global import - case of named import', () => { + env.write('comp1.ts', ` + import {Component} from '@angular/core'; + + @Component({template:''}) + export class Comp1 { + } + `); + env.write('module1.ts', ` + import {NgModule} from '@angular/core'; + + import {Comp1} from 'comp1'; + + @NgModule({declarations:[Comp1]}) + export class Module1 { + } + `); env.write('a.ts', ` import {NgModule} from '@angular/core'; import {SomeExternalStuff} from '/some_external_file'; @@ -143,19 +260,31 @@ runInEachFileSystem(() => { @NgModule({imports: [[[SomeExternalStuff]]]}) export class AModule { } - `); - env.write('test.ts', ` - // Some code `); env.driveMain(); - expect(env.getContents('a.js')).toContain('import "/some_external_file"'); - expect(env.getContents('test.js')).toContain('import "/some_external_file"'); + expect(env.getContents('comp1.js')).toContain('import "/some_external_file"'); }); it('should include nested NgModule external import as global import - case of namespace import', () => { + env.write('comp1.ts', ` + import {Component} from '@angular/core'; + + @Component({template:''}) + export class Comp1 { + } + `); + env.write('module1.ts', ` + import {NgModule} from '@angular/core'; + + import {Comp1} from 'comp1'; + + @NgModule({declarations:[Comp1]}) + export class Module1 { + } + `); env.write('a.ts', ` import {NgModule} from '@angular/core'; import * as n from '/some_external_file'; @@ -165,19 +294,31 @@ runInEachFileSystem(() => { @NgModule({imports: [[[n.SomeExternalStuff]]]}) export class AModule { } - `); - env.write('test.ts', ` - // Some code `); env.driveMain(); - expect(env.getContents('a.js')).toContain('import "/some_external_file"'); - expect(env.getContents('test.js')).toContain('import "/some_external_file"'); + expect(env.getContents('comp1.js')).toContain('import "/some_external_file"'); }); it('should include NgModule external imports as global imports - case of multiple nested imports including named and namespace imports', () => { + env.write('comp1.ts', ` + import {Component} from '@angular/core'; + + @Component({template:''}) + export class Comp1 { + } + `); + env.write('module1.ts', ` + import {NgModule} from '@angular/core'; + + import {Comp1} from 'comp1'; + + @NgModule({declarations:[Comp1]}) + export class Module1 { + } + `); env.write('a.ts', ` import {NgModule} from '@angular/core'; import {SomeExternalStuff} from '/some_external_file'; @@ -189,14 +330,12 @@ runInEachFileSystem(() => { export class AModule { } `); - env.write('test.ts', ` - // Some code - `); + env.driveMain(); - expect(env.getContents('test.js')).toContain('import "/some_external_file"'); - expect(env.getContents('test.js')).toContain('import "/some_external_file2"'); + expect(env.getContents('comp1.js')).toContain('import "/some_external_file"'); + expect(env.getContents('comp1.js')).toContain('import "/some_external_file2"'); }); it('should include extra import for the local component dependencies (component, directive and pipe)',