Skip to content

Commit 8d8c03a

Browse files
crisbetoAndrewKushnir
authored andcommitted
fix(compiler-cli): defer symbols only used in types (angular#58104)
Currently we don't defer any symbols that have references outside of the `import` statement and the `imports` array. This is a bit too aggressive, because it's possible that the symbol is only used for types (e.g. `viewChild<SomeCmp>('ref')`) which will be stripped when emitting to JS. These changes expand the logic so that references inside type nodes aren't considered. **Note:** one special case is when the symbol used in constructor-based DI (e.g. `constructor(someCmp: SomeCmp)`, because these constructors will be compiled to `directiveInject` calls. We don't need to worry about them, because the compiler introduces an addition `import * as i1 from './some-cmp';` import that it uses to refer to the symbol. Fixes angular#55991. PR Close angular#58104
1 parent 5c61f46 commit 8d8c03a

File tree

2 files changed

+141
-37
lines changed

2 files changed

+141
-37
lines changed

packages/compiler-cli/src/ngtsc/imports/src/deferred_symbol_tracker.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ export class DeferredSymbolTracker {
171171
}
172172

173173
const symbolsMap = this.imports.get(importDecl)!;
174-
for (const [symbol, refs] of symbolsMap) {
174+
for (const refs of symbolsMap.values()) {
175175
if (refs === AssumeEager || refs.size > 0) {
176176
// There may be still eager references to this symbol.
177177
return false;
@@ -201,8 +201,9 @@ export class DeferredSymbolTracker {
201201
): Set<ts.Identifier> {
202202
const results = new Set<ts.Identifier>();
203203
const visit = (node: ts.Node): void => {
204-
if (node === importDecl) {
205-
// Don't record references from the declaration itself.
204+
// Don't record references from the declaration itself or inside
205+
// type nodes which will be stripped from the JS output.
206+
if (node === importDecl || ts.isTypeNode(node)) {
206207
return;
207208
}
208209

packages/compiler-cli/test/ngtsc/defer_spec.ts

Lines changed: 137 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -79,12 +79,10 @@ runInEachFileSystem(() => {
7979
expect(jsContents).not.toContain('import { CmpA }');
8080
});
8181

82-
it(
83-
'should include timer scheduler function when ' + '`after` or `minimum` parameters are used',
84-
() => {
85-
env.write(
86-
'cmp-a.ts',
87-
`
82+
it('should include timer scheduler function when `after` or `minimum` parameters are used', () => {
83+
env.write(
84+
'cmp-a.ts',
85+
`
8886
import { Component } from '@angular/core';
8987
9088
@Component({
@@ -94,38 +92,37 @@ runInEachFileSystem(() => {
9492
})
9593
export class CmpA {}
9694
`,
97-
);
95+
);
9896

99-
env.write(
100-
'/test.ts',
101-
`
102-
import { Component } from '@angular/core';
103-
import { CmpA } from './cmp-a';
97+
env.write(
98+
'/test.ts',
99+
`
100+
import { Component } from '@angular/core';
101+
import { CmpA } from './cmp-a';
104102
105-
@Component({
106-
selector: 'test-cmp',
107-
standalone: true,
108-
imports: [CmpA],
109-
template: \`
110-
@defer {
111-
<cmp-a />
112-
} @loading (after 500ms; minimum 300ms) {
113-
Loading...
114-
}
115-
\`,
116-
})
117-
export class TestCmp {}
118-
`,
119-
);
103+
@Component({
104+
selector: 'test-cmp',
105+
standalone: true,
106+
imports: [CmpA],
107+
template: \`
108+
@defer {
109+
<cmp-a />
110+
} @loading (after 500ms; minimum 300ms) {
111+
Loading...
112+
}
113+
\`,
114+
})
115+
export class TestCmp {}
116+
`,
117+
);
120118

121-
env.driveMain();
119+
env.driveMain();
122120

123-
const jsContents = env.getContents('test.js');
124-
expect(jsContents).toContain(
125-
'ɵɵdefer(2, 0, TestCmp_Defer_2_DepsFn, 1, null, null, 0, null, i0.ɵɵdeferEnableTimerScheduling)',
126-
);
127-
},
128-
);
121+
const jsContents = env.getContents('test.js');
122+
expect(jsContents).toContain(
123+
'ɵɵdefer(2, 0, TestCmp_Defer_2_DepsFn, 1, null, null, 0, null, i0.ɵɵdeferEnableTimerScheduling)',
124+
);
125+
});
129126

130127
describe('imports', () => {
131128
it('should retain regular imports when symbol is eagerly referenced', () => {
@@ -652,6 +649,112 @@ runInEachFileSystem(() => {
652649
// via dynamic imports and an original import can be removed.
653650
expect(jsContents).not.toContain('import CmpA');
654651
});
652+
653+
it('should defer symbol that is used only in types', () => {
654+
env.write(
655+
'cmp.ts',
656+
`
657+
import { Component } from '@angular/core';
658+
659+
@Component({
660+
standalone: true,
661+
selector: 'cmp',
662+
template: 'Cmp!'
663+
})
664+
export class Cmp {}
665+
`,
666+
);
667+
668+
env.write(
669+
'/test.ts',
670+
`
671+
import { Component, viewChild } from '@angular/core';
672+
import { Cmp } from './cmp';
673+
674+
const topLevelConst: Cmp = null!;
675+
676+
@Component({
677+
standalone: true,
678+
imports: [Cmp],
679+
template: \`
680+
@defer {
681+
<cmp #ref/>
682+
}
683+
\`,
684+
})
685+
export class TestCmp {
686+
query = viewChild<Cmp>('ref');
687+
asType: Cmp;
688+
inlineType: {foo: Cmp};
689+
unionType: string | Cmp | number;
690+
constructor(param: Cmp) {}
691+
inMethod(param: Cmp): Cmp {
692+
let localVar: Cmp | null = null;
693+
return localVar!;
694+
}
695+
}
696+
697+
function inFunction(param: Cmp): Cmp {
698+
return null!;
699+
}
700+
`,
701+
);
702+
703+
env.driveMain();
704+
705+
const jsContents = env.getContents('test.js');
706+
expect(jsContents).toContain('ɵɵdefer(1, 0, TestCmp_Defer_1_DepsFn)');
707+
expect(jsContents).toContain('() => [import("./cmp").then(m => m.Cmp)]');
708+
expect(jsContents).not.toContain('import { Cmp }');
709+
});
710+
711+
it('should retain symbols used in types and eagerly', () => {
712+
env.write(
713+
'cmp.ts',
714+
`
715+
import { Component } from '@angular/core';
716+
717+
@Component({
718+
standalone: true,
719+
selector: 'cmp',
720+
template: 'Cmp!'
721+
})
722+
export class Cmp {}
723+
`,
724+
);
725+
726+
env.write(
727+
'/test.ts',
728+
`
729+
import { Component, viewChild } from '@angular/core';
730+
import { Cmp } from './cmp';
731+
732+
@Component({
733+
standalone: true,
734+
imports: [Cmp],
735+
template: \`
736+
@defer {
737+
<cmp #ref/>
738+
}
739+
\`,
740+
})
741+
export class TestCmp {
742+
// Type-only reference
743+
query = viewChild<Cmp>('ref');
744+
745+
// Directy reference
746+
otherQuery = viewChild(Cmp);
747+
}
748+
`,
749+
);
750+
751+
env.driveMain();
752+
753+
const jsContents = env.getContents('test.js');
754+
expect(jsContents).toContain('ɵɵdefer(1, 0, TestCmp_Defer_1_DepsFn)');
755+
expect(jsContents).toContain('() => [Cmp]');
756+
expect(jsContents).toContain('import { Cmp }');
757+
});
655758
});
656759

657760
it('should detect pipe used in the `when` trigger as an eager dependency', () => {

0 commit comments

Comments
 (0)