Skip to content

Commit

Permalink
fix(compiler-cli): exclude type-only imports from cycle analysis (#42453
Browse files Browse the repository at this point in the history
)

Type-only imports are known to be elided by TypeScript, so the compiler
can be certain that such imports do not contribute to potential import
cycles. As such, type-only imports are no longer considered during cycle
analysis.

Regular import statements that would eventually be fully elided by
TypeScript during emit if none of the imported symbols are used in a
value position continue to be included in the cycle analysis, as the
cycle analyzer is unaware of these elision opportunities. Only explicit
`import type` statements are excluded.

PR Close #42453
  • Loading branch information
JoostK authored and thePunderWoman committed Jun 3, 2021
1 parent 0dad375 commit b6d6a34
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 0 deletions.
7 changes: 7 additions & 0 deletions packages/compiler-cli/src/ngtsc/cycles/src/imports.ts
Expand Up @@ -110,6 +110,13 @@ export class ImportGraph {
continue;
}

if (ts.isImportDeclaration(stmt) && stmt.importClause !== undefined &&
stmt.importClause.isTypeOnly) {
// Exclude type-only imports as they are always elided, so they don't contribute to
// cycles.
continue;
}

const symbol = this.checker.getSymbolAtLocation(stmt.moduleSpecifier);
if (symbol === undefined || symbol.valueDeclaration === undefined) {
// No symbol could be found to skip over this import/export.
Expand Down
11 changes: 11 additions & 0 deletions packages/compiler-cli/src/ngtsc/cycles/test/analyzer_spec.ts
Expand Up @@ -70,6 +70,17 @@ runInEachFileSystem(() => {
expect(cycle).toBeInstanceOf(Cycle);
expect(importPath(cycle!.getPath())).toEqual('b,c,b');
});

it('should not consider type-only imports', () => {
const {program, analyzer} = makeAnalyzer('a:b,c!;b;c');
const a = getSourceFileOrError(program, (_('/a.ts')));
const b = getSourceFileOrError(program, (_('/b.ts')));
const c = getSourceFileOrError(program, (_('/c.ts')));
expect(analyzer.wouldCreateCycle(c, a)).toBe(null);
const cycle = analyzer.wouldCreateCycle(b, a);
expect(cycle).toBeInstanceOf(Cycle);
expect(importPath(cycle!.getPath())).toEqual('b,a,b');
});
});

function makeAnalyzer(graph: string): {program: ts.Program, analyzer: CycleAnalyzer} {
Expand Down
5 changes: 5 additions & 0 deletions packages/compiler-cli/src/ngtsc/cycles/test/util.ts
Expand Up @@ -30,6 +30,8 @@ import {makeProgram} from '../../testing';
* "a:*b,c;b;c"
*
* represents a program where a.ts exports from b.ts and imports from c.ts.
*
* An import can be suffixed with ! to make it a type-only import.
*/
export function makeProgramFromGraph(fs: PathManipulation, graph: string): {
program: ts.Program,
Expand All @@ -43,6 +45,9 @@ export function makeProgramFromGraph(fs: PathManipulation, graph: string): {
if (i.startsWith('*')) {
const sym = i.substr(1);
return `export {${sym}} from './${sym}';`;
} else if (i.endsWith('!')) {
const sym = i.substr(0, i.length - 1);
return `import type {${sym}} from './${sym}';`;
} else {
return `import {${i}} from './${i}';`;
}
Expand Down
35 changes: 35 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -4836,6 +4836,41 @@ function allTests(os: string) {
expect(jsContents).not.toContain('setComponentScope');
});

it('should not consider type-only imports during cycle detection', () => {
env.write('test.ts', `
import {NgModule} from '@angular/core';
import {ACmp} from './a';
import {BCmp} from './b';
@NgModule({declarations: [ACmp, BCmp]})
export class Module {}
`);
env.write('a.ts', `
import {Component} from '@angular/core';
@Component({
selector: 'a-cmp',
template: '<b-cmp></b-cmp>',
})
export class ACmp {}
`);
env.write('b.ts', `
import {Component} from '@angular/core';
import type {ACmp} from './a';
@Component({
selector: 'b-cmp',
template: 'does not use a-cmp',
})
export class BCmp {
a: ACmp;
}
`);
env.driveMain();
const jsContents = env.getContents('test.js');
expect(jsContents).not.toContain('setComponentScope');
});

it('should only pass components actually used to setComponentScope', () => {
env.write('test.ts', `
import {Component, NgModule} from '@angular/core';
Expand Down

0 comments on commit b6d6a34

Please sign in to comment.