Skip to content

Commit

Permalink
refactor(compiler-cli): optimize extra import generation in local com…
Browse files Browse the repository at this point in the history 
…pilation 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.
  • Loading branch information
@pmvald
pmvald committed Apr 25, 2024
1 parent e478180 commit 0fbf375
Show file tree
Hide file tree
Showing 3 changed files with 199 additions and 35 deletions.
6 changes: 6 additions & 0 deletions packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts
View file
Expand Up @@ -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 &&
Expand Down
19 changes: 19 additions & 0 deletions packages/compiler-cli/src/ngtsc/imports/src/local_compilation_extra_imports_tracker.ts
View file
Expand Up @@ -37,8 +37,23 @@ export class LocalCompilationExtraImportsTracker {
private readonly localImportsMap = new Map<string, Set<string>>();
private readonly globalImportsSet = new Set<string>();

/** Names of the files marked for extra import generation. */
private readonly markedFilesSet = new Set<string>();

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.
*/
Expand Down Expand Up @@ -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) ?? []),
Expand Down
209 changes: 174 additions & 35 deletions packages/compiler-cli/test/ngtsc/local_compilation_spec.ts
View file
Expand Up @@ -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';
Expand All @@ -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';
Expand All @@ -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';
Expand All @@ -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';
Expand All @@ -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';
Expand All @@ -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)',
Expand Down

0 comments on commit 0fbf375

Please sign in to comment.