Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(compiler-cli): optimize extra import generation in local com… #55548

Closed
wants to merge 1 commit into from
Closed

refactor(compiler-cli): optimize extra import generation in local com… #55548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts
View file
Open in desktop
Expand Up @@ -1058,6 +1058,12 @@ export class ComponentDecoratorHandler
// 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 (
Expand Down
19 changes: 19 additions & 0 deletions packages/compiler-cli/src/ngtsc/imports/src/local_compilation_extra_imports_tracker.ts
View file
Open in desktop
Expand Up @@ -36,8 +36,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 @@ -89,6 +104,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
265 changes: 222 additions & 43 deletions packages/compiler-cli/test/ngtsc/local_compilation_spec.ts
View file
Open in desktop
Expand Up @@ -71,6 +71,116 @@ 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',
`
Expand Down Expand Up @@ -103,27 +213,51 @@ 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',
`
Expand All @@ -146,11 +280,32 @@ 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',
`
Expand All @@ -164,20 +319,35 @@ runInEachFileSystem(() => {
}
`,
);
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',
`
Expand All @@ -191,20 +361,35 @@ runInEachFileSystem(() => {
}
`,
);
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',
`
Expand All @@ -219,17 +404,11 @@ runInEachFileSystem(() => {
}
`,
);
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