Skip to content

Commit a07b9bb

Browse files
crisbetoPrajaktaB27
authored andcommitted
fix(compiler-cli): disable tree shaking during HMR (angular#59595)
When HMR is enabled, we need to capture the dependencies used in a template and forward them to the HMR replacement function. One half of this process is static, meaning that we can't change it after the initial compilation. Tree shaking becomes a problem in such a case, because the user can change the template in a way that changes the set of dependencies which will start matching with the static part of the HMR code. These changes disable the tree shaking when HMR is enabled to ensure that the dependencies stay stable. Fixes angular#59581. PR Close angular#59595
1 parent 9b913f8 commit a07b9bb

File tree

2 files changed

+167
-8
lines changed

2 files changed

+167
-8
lines changed

packages/compiler-cli/src/ngtsc/annotations/component/src/handler.ts

Lines changed: 18 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1222,14 +1222,25 @@ export class ComponentDecoratorHandler
12221222
// Register all Directives and Pipes used at the top level (outside
12231223
// of any defer blocks), which would be eagerly referenced.
12241224
const eagerlyUsed = new Set<ClassDeclaration>();
1225-
for (const dir of bound.getEagerlyUsedDirectives()) {
1226-
eagerlyUsed.add(dir.ref.node);
1227-
}
1228-
for (const name of bound.getEagerlyUsedPipes()) {
1229-
if (!pipes.has(name)) {
1230-
continue;
1225+
1226+
if (this.enableHmr) {
1227+
// In HMR we need to preserve all the dependencies, because they have to remain consistent
1228+
// with the initially-generated code no matter what the template looks like.
1229+
for (const dep of dependencies) {
1230+
if (dep.ref.node !== node) {
1231+
eagerlyUsed.add(dep.ref.node);
1232+
}
1233+
}
1234+
} else {
1235+
for (const dir of bound.getEagerlyUsedDirectives()) {
1236+
eagerlyUsed.add(dir.ref.node);
1237+
}
1238+
for (const name of bound.getEagerlyUsedPipes()) {
1239+
if (!pipes.has(name)) {
1240+
continue;
1241+
}
1242+
eagerlyUsed.add(pipes.get(name)!.ref.node);
12311243
}
1232-
eagerlyUsed.add(pipes.get(name)!.ref.node);
12331244
}
12341245

12351246
// Set of Directives and Pipes used across the entire template,

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

Lines changed: 149 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ runInEachFileSystem(() => {
291291
expect(jsContents).toContain('i0.ɵɵdefer(1, 0, Cmp_Defer_1_DepsFn);');
292292

293293
expect(hmrContents).toContain(
294-
'export default function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces, Component, Dep) {',
294+
'export default function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces, Dep, Component) {',
295295
);
296296
expect(hmrContents).toContain('const Cmp_Defer_1_DepsFn = () => [Dep];');
297297
expect(hmrContents).toContain('function Cmp_Defer_0_Template(rf, ctx) {');
@@ -502,5 +502,153 @@ runInEachFileSystem(() => {
502502
'export default function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces, token, value, Component) {',
503503
);
504504
});
505+
506+
it('should preserve eager standalone imports in HMR even if they are not used in the template', () => {
507+
enableHmr({
508+
// Disable class metadata since it can add noise to the test.
509+
supportTestBed: false,
510+
extendedDiagnostics: {
511+
checks: {
512+
// Disable the diagnostic that flags standalone imports since
513+
// we need one to simulate the case we're looking for.
514+
unusedStandaloneImports: 'suppress',
515+
},
516+
},
517+
});
518+
519+
env.write(
520+
'dep.ts',
521+
`
522+
import {Directive} from '@angular/core';
523+
524+
@Directive({selector: '[dep]'})
525+
export class Dep {}
526+
`,
527+
);
528+
529+
env.write(
530+
'test.ts',
531+
`
532+
import {Component} from '@angular/core';
533+
import {Dep} from './dep';
534+
535+
@Component({
536+
selector: 'cmp',
537+
template: '',
538+
imports: [Dep],
539+
})
540+
export class Cmp {}
541+
`,
542+
);
543+
544+
env.driveMain();
545+
546+
const jsContents = env.getContents('test.js');
547+
const hmrContents = env.driveHmr('test.ts', 'Cmp');
548+
549+
expect(jsContents).toContain('dependencies: [Dep]');
550+
expect(jsContents).toContain('ɵɵreplaceMetadata(Cmp, m.default, [i0], [Dep]));');
551+
expect(hmrContents).toContain('function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces, Dep) {');
552+
});
553+
554+
it('should preserve eager module imports inside standalone component in HMR even if they are not used in the template', () => {
555+
enableHmr({
556+
// Disable class metadata since it can add noise to the test.
557+
supportTestBed: false,
558+
});
559+
560+
env.write(
561+
'dep.ts',
562+
`
563+
import {NgModule, Directive} from '@angular/core';
564+
565+
@Directive({selector: '[dep]', standalone: false})
566+
export class Dep {}
567+
568+
@NgModule({declarations: [Dep], exports: [Dep]})
569+
export class DepModule {}
570+
`,
571+
);
572+
573+
env.write(
574+
'test.ts',
575+
`
576+
import {Component} from '@angular/core';
577+
import {DepModule} from './dep';
578+
579+
@Component({
580+
selector: 'cmp',
581+
template: '',
582+
imports: [DepModule],
583+
})
584+
export class Cmp {}
585+
`,
586+
);
587+
588+
env.driveMain();
589+
590+
const jsContents = env.getContents('test.js');
591+
const hmrContents = env.driveHmr('test.ts', 'Cmp');
592+
593+
expect(jsContents).toContain('dependencies: [DepModule, i1.Dep]');
594+
expect(jsContents).toContain('ɵɵreplaceMetadata(Cmp, m.default, [i0, i1], [DepModule]));');
595+
expect(hmrContents).toContain('function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces, DepModule) {');
596+
});
597+
598+
it('should preserve eager module imports inside non-standalone component in HMR even if they are not used in the template', () => {
599+
enableHmr({
600+
// Disable class metadata since it can add noise to the test.
601+
supportTestBed: false,
602+
});
603+
604+
env.write(
605+
'dep.ts',
606+
`
607+
import {NgModule, Directive} from '@angular/core';
608+
609+
@Directive({selector: '[dep]', standalone: false})
610+
export class Dep {}
611+
612+
@NgModule({declarations: [Dep], exports: [Dep]})
613+
export class DepModule {}
614+
`,
615+
);
616+
617+
env.write(
618+
'test-module.ts',
619+
`
620+
import {NgModule} from '@angular/core';
621+
import {Cmp} from './test';
622+
import {DepModule} from './dep';
623+
624+
@NgModule({imports: [DepModule], declarations: [Cmp], exports: [Cmp]})
625+
export class CmpModule {}
626+
`,
627+
);
628+
629+
env.write(
630+
'test.ts',
631+
`
632+
import {Component} from '@angular/core';
633+
import {DepModule} from './dep';
634+
635+
@Component({
636+
selector: 'cmp',
637+
template: '',
638+
standalone: false,
639+
})
640+
export class Cmp {}
641+
`,
642+
);
643+
644+
env.driveMain();
645+
646+
const jsContents = env.getContents('test.js');
647+
const hmrContents = env.driveHmr('test.ts', 'Cmp');
648+
649+
expect(jsContents).toContain('dependencies: [i1.Dep]');
650+
expect(jsContents).toContain('ɵɵreplaceMetadata(Cmp, m.default, [i0, i1], []));');
651+
expect(hmrContents).toContain('function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces) {');
652+
});
505653
});
506654
});

0 commit comments

Comments
 (0)