Skip to content

Commit 53a4668

Browse files
crisbetoalxhub
crisbeto
authored and
alxhub
committed
fix(compiler-cli): handle const enums used inside HMR data (#59815)
When we generate an HMR replacement function, we determine which locals from the file are used and we pass them by reference. This works fine in most cases, but breaks down for const enums which don't have a runtime representation. These changes work around the issue by passing in all the values as an object literal. Fixes #59800. PR Close #59815
1 parent 976125e commit 53a4668

File tree

6 files changed

+201
-15
lines changed

6 files changed

+201
-15
lines changed

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1659,6 +1659,7 @@ export class ComponentDecoratorHandler
16591659
? extractHmrMetatadata(
16601660
node,
16611661
this.reflector,
1662+
this.evaluator,
16621663
this.compilerHost,
16631664
this.rootDirs,
16641665
def,
@@ -1725,6 +1726,7 @@ export class ComponentDecoratorHandler
17251726
? extractHmrMetatadata(
17261727
node,
17271728
this.reflector,
1729+
this.evaluator,
17281730
this.compilerHost,
17291731
this.rootDirs,
17301732
def,
@@ -1787,6 +1789,7 @@ export class ComponentDecoratorHandler
17871789
? extractHmrMetatadata(
17881790
node,
17891791
this.reflector,
1792+
this.evaluator,
17901793
this.compilerHost,
17911794
this.rootDirs,
17921795
def,
@@ -1843,6 +1846,7 @@ export class ComponentDecoratorHandler
18431846
? extractHmrMetatadata(
18441847
node,
18451848
this.reflector,
1849+
this.evaluator,
18461850
this.compilerHost,
18471851
this.rootDirs,
18481852
def,

‎packages/compiler-cli/src/ngtsc/hmr/BUILD.bazel‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ ts_library(
99
]),
1010
deps = [
1111
"//packages/compiler",
12+
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
1213
"//packages/compiler-cli/src/ngtsc/reflection",
1314
"//packages/compiler-cli/src/ngtsc/transform",
1415
"//packages/compiler-cli/src/ngtsc/translator",

‎packages/compiler-cli/src/ngtsc/hmr/src/extract_dependencies.ts‎

Lines changed: 92 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,10 @@ import {
1313
R3HmrNamespaceDependency,
1414
outputAst as o,
1515
} from '@angular/compiler';
16-
import {DeclarationNode} from '../../reflection';
16+
import {DeclarationNode, ReflectionHost} from '../../reflection';
1717
import {CompileResult} from '../../transform';
1818
import ts from 'typescript';
19+
import {EnumValue, PartialEvaluator} from '../../partial_evaluator';
1920

2021
/**
2122
* Determines the file-level dependencies that the HMR initializer needs to capture and pass along.
@@ -33,7 +34,12 @@ export function extractHmrDependencies(
3334
deferBlockMetadata: R3ComponentDeferMetadata,
3435
classMetadata: o.Statement | null,
3536
debugInfo: o.Statement | null,
36-
): {local: string[]; external: R3HmrNamespaceDependency[]} {
37+
reflection: ReflectionHost,
38+
evaluator: PartialEvaluator,
39+
): {
40+
local: {name: string; runtimeRepresentation: o.Expression}[];
41+
external: R3HmrNamespaceDependency[];
42+
} {
3743
const name = ts.isClassDeclaration(node) && node.name ? node.name.text : null;
3844
const visitor = new PotentialTopLevelReadsVisitor();
3945
const sourceFile = node.getSourceFile();
@@ -57,16 +63,73 @@ export function extractHmrDependencies(
5763
// variables inside of functions. Note that we filter out the class name since it is always
5864
// defined and it saves us having to repeat this logic wherever the locals are consumed.
5965
const availableTopLevel = getTopLevelDeclarationNames(sourceFile);
66+
const local: {name: string; runtimeRepresentation: o.Expression}[] = [];
67+
const seenLocals = new Set<string>();
68+
69+
for (const readNode of visitor.allReads) {
70+
const readName = readNode instanceof o.ReadVarExpr ? readNode.name : readNode.text;
71+
72+
if (readName !== name && !seenLocals.has(readName) && availableTopLevel.has(readName)) {
73+
local.push({
74+
name: readName,
75+
runtimeRepresentation: getRuntimeRepresentation(readNode, reflection, evaluator),
76+
});
77+
seenLocals.add(readName);
78+
}
79+
}
6080

6181
return {
62-
local: Array.from(visitor.allReads).filter((r) => r !== name && availableTopLevel.has(r)),
82+
local,
6383
external: Array.from(visitor.namespaceReads, (name, index) => ({
6484
moduleName: name,
6585
assignedName: `ɵhmr${index}`,
6686
})),
6787
};
6888
}
6989

90+
/**
91+
* Gets a node that can be used to represent an identifier in the HMR replacement code at runtime.
92+
*/
93+
function getRuntimeRepresentation(
94+
node: o.ReadVarExpr | ts.Identifier,
95+
reflection: ReflectionHost,
96+
evaluator: PartialEvaluator,
97+
): o.Expression {
98+
if (node instanceof o.ReadVarExpr) {
99+
return o.variable(node.name);
100+
}
101+
102+
// Const enums can't be passed by reference, because their values are inlined.
103+
// Pass in an object literal with all of the values instead.
104+
if (isConstEnumReference(node, reflection)) {
105+
const evaluated = evaluator.evaluate(node);
106+
107+
if (evaluated instanceof Map) {
108+
const members: {key: string; quoted: boolean; value: o.Expression}[] = [];
109+
110+
for (const [name, value] of evaluated.entries()) {
111+
if (
112+
value instanceof EnumValue &&
113+
(value.resolved == null ||
114+
typeof value.resolved === 'string' ||
115+
typeof value.resolved === 'boolean' ||
116+
typeof value.resolved === 'number')
117+
) {
118+
members.push({
119+
key: name,
120+
quoted: false,
121+
value: o.literal(value.resolved),
122+
});
123+
}
124+
}
125+
126+
return o.literalMap(members);
127+
}
128+
}
129+
130+
return o.variable(node.text);
131+
}
132+
70133
/**
71134
* Gets the names of all top-level declarations within the file (imports, declared classes etc).
72135
* @param sourceFile File in which to search for locals.
@@ -81,8 +144,7 @@ function getTopLevelDeclarationNames(sourceFile: ts.SourceFile): Set<string> {
81144
if (
82145
ts.isClassDeclaration(node) ||
83146
ts.isFunctionDeclaration(node) ||
84-
(ts.isEnumDeclaration(node) &&
85-
!node.modifiers?.some((m) => m.kind === ts.SyntaxKind.ConstKeyword))
147+
ts.isEnumDeclaration(node)
86148
) {
87149
if (node.name) {
88150
results.add(node.name.text);
@@ -157,7 +219,7 @@ function trackBindingName(node: ts.BindingName, results: Set<string>): void {
157219
* inside functions.
158220
*/
159221
class PotentialTopLevelReadsVisitor extends o.RecursiveAstVisitor {
160-
readonly allReads = new Set<string>();
222+
readonly allReads = new Set<o.ReadVarExpr | ts.Identifier>();
161223
readonly namespaceReads = new Set<string>();
162224

163225
override visitExternalExpr(ast: o.ExternalExpr, context: any) {
@@ -168,7 +230,7 @@ class PotentialTopLevelReadsVisitor extends o.RecursiveAstVisitor {
168230
}
169231

170232
override visitReadVarExpr(ast: o.ReadVarExpr, context: any) {
171-
this.allReads.add(ast.name);
233+
this.allReads.add(ast);
172234
super.visitReadVarExpr(ast, context);
173235
}
174236

@@ -186,7 +248,7 @@ class PotentialTopLevelReadsVisitor extends o.RecursiveAstVisitor {
186248
*/
187249
private addAllTopLevelIdentifiers = (node: ts.Node) => {
188250
if (ts.isIdentifier(node) && this.isTopLevelIdentifierReference(node)) {
189-
this.allReads.add(node.text);
251+
this.allReads.add(node);
190252
} else {
191253
ts.forEachChild(node, this.addAllTopLevelIdentifiers);
192254
}
@@ -326,3 +388,25 @@ class PotentialTopLevelReadsVisitor extends o.RecursiveAstVisitor {
326388
return !!value && typeof value.kind === 'number';
327389
}
328390
}
391+
392+
/** Checks whether a node is a reference to a const enum. */
393+
function isConstEnumReference(node: ts.Identifier, reflection: ReflectionHost): boolean {
394+
const parent = node.parent;
395+
396+
// Only check identifiers that are in the form of `Foo.bar` where `Foo` is the node being checked.
397+
if (
398+
!parent ||
399+
!ts.isPropertyAccessExpression(parent) ||
400+
parent.expression !== node ||
401+
!ts.isIdentifier(parent.name)
402+
) {
403+
return false;
404+
}
405+
406+
const declaration = reflection.getDeclarationOfIdentifier(node);
407+
return (
408+
declaration !== null &&
409+
ts.isEnumDeclaration(declaration.node) &&
410+
!!declaration.node.modifiers?.some((m) => m.kind === ts.SyntaxKind.ConstKeyword)
411+
);
412+
}

‎packages/compiler-cli/src/ngtsc/hmr/src/metadata.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {getProjectRelativePath} from '../../util/src/path';
1717
import {CompileResult} from '../../transform';
1818
import {extractHmrDependencies} from './extract_dependencies';
1919
import ts from 'typescript';
20+
import {PartialEvaluator} from '../../partial_evaluator';
2021

2122
/**
2223
* Extracts the HMR metadata for a class declaration.
@@ -33,6 +34,7 @@ import ts from 'typescript';
3334
export function extractHmrMetatadata(
3435
clazz: DeclarationNode,
3536
reflection: ReflectionHost,
37+
evaluator: PartialEvaluator,
3638
compilerHost: Pick<ts.CompilerHost, 'getCanonicalFileName'>,
3739
rootDirs: readonly string[],
3840
definition: R3CompiledExpression,
@@ -57,6 +59,8 @@ export function extractHmrMetatadata(
5759
deferBlockMetadata,
5860
classMetadata,
5961
debugInfo,
62+
reflection,
63+
evaluator,
6064
);
6165
const meta: R3HmrMetadata = {
6266
type: new o.WrappedNodeExpr(clazz.name),

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

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -800,5 +800,92 @@ runInEachFileSystem(() => {
800800
expect(jsContents).toContain('ɵɵreplaceMetadata(Cmp, m.default, [i0, i1], []));');
801801
expect(hmrContents).toContain('function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces) {');
802802
});
803+
804+
it('should pass const enums defined in the same file as an object literal', () => {
805+
enableHmr();
806+
env.write(
807+
'test.ts',
808+
`
809+
import {Component, InjectionToken} from '@angular/core';
810+
811+
const token = new InjectionToken<number>('TEST');
812+
813+
const numberThree = 3;
814+
815+
export const enum Foo {
816+
one,
817+
two = '2',
818+
three = numberThree
819+
}
820+
821+
@Component({
822+
template: '',
823+
providers: [{
824+
provide: token,
825+
useValue: Foo.three
826+
}]
827+
})
828+
export class Cmp {}
829+
`,
830+
);
831+
832+
env.driveMain();
833+
834+
const jsContents = env.getContents('test.js');
835+
const hmrContents = env.driveHmr('test.ts', 'Cmp');
836+
expect(jsContents).toContain(
837+
'ɵɵreplaceMetadata(Cmp, m.default, [i0], [token, { one: 0, two: "2", three: 3 }, Component]));',
838+
);
839+
expect(hmrContents).toContain(
840+
'export default function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces, token, Foo, Component) {',
841+
);
842+
});
843+
844+
it('should pass const enum defined in other file as an object literal', () => {
845+
enableHmr();
846+
847+
env.write(
848+
'deps.ts',
849+
`
850+
const numberThree = 3;
851+
852+
export const enum Foo {
853+
one,
854+
two = '2',
855+
three = numberThree
856+
}
857+
`,
858+
);
859+
860+
env.write(
861+
'test.ts',
862+
`
863+
import {Component, InjectionToken} from '@angular/core';
864+
import {Foo} from './deps';
865+
866+
const token = new InjectionToken<number>('TEST');
867+
868+
@Component({
869+
template: '',
870+
providers: [{
871+
provide: token,
872+
useValue: Foo.three
873+
}]
874+
})
875+
export class Cmp {}
876+
`,
877+
);
878+
879+
env.driveMain();
880+
881+
const jsContents = env.getContents('test.js');
882+
const hmrContents = env.driveHmr('test.ts', 'Cmp');
883+
expect(jsContents).toContain(
884+
'ɵɵreplaceMetadata(Cmp, m.default, [i0], [token, { one: 0, two: "2", three: 3 }, Component]));',
885+
);
886+
expect(hmrContents).toContain(
887+
'export default function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces, token, Foo, Component) {',
888+
);
889+
});
803890
});
804891
});

‎packages/compiler/src/render3/r3_hmr_compiler.ts‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@ export interface R3HmrMetadata {
3131
/**
3232
* HMR update functions cannot contain imports so any locals the generated code depends on
3333
* (e.g. references to imports within the same file or imported symbols) have to be passed in
34-
* as function parameters. This array contains the names of those local symbols.
34+
* as function parameters. This array contains the names and runtime representation of the locals.
3535
*/
36-
localDependencies: string[];
36+
localDependencies: {name: string; runtimeRepresentation: o.Expression}[];
3737
}
3838

3939
/** HMR dependency on a namespace import. */
@@ -59,7 +59,6 @@ export function compileHmrInitializer(meta: R3HmrMetadata): o.Expression {
5959
const dataName = 'd';
6060
const timestampName = 't';
6161
const importCallbackName = `${meta.className}_HmrLoad`;
62-
const locals = meta.localDependencies.map((localName) => o.variable(localName));
6362
const namespaces = meta.namespaceDependencies.map((dep) => {
6463
return new o.ExternalExpr({moduleName: dep.moduleName, name: null});
6564
});
@@ -70,7 +69,12 @@ export function compileHmrInitializer(meta: R3HmrMetadata): o.Expression {
7069
// ɵɵreplaceMetadata(Comp, m.default, [...namespaces], [...locals]);
7170
const replaceCall = o
7271
.importExpr(R3.replaceMetadata)
73-
.callFn([meta.type, defaultRead, o.literalArr(namespaces), o.literalArr(locals)]);
72+
.callFn([
73+
meta.type,
74+
defaultRead,
75+
o.literalArr(namespaces),
76+
o.literalArr(meta.localDependencies.map((l) => l.runtimeRepresentation)),
77+
]);
7478

7579
// (m) => m.default && ɵɵreplaceMetadata(...)
7680
const replaceCallback = o.arrowFn([new o.FnParam(moduleName)], defaultRead.and(replaceCall));
@@ -159,11 +163,13 @@ export function compileHmrUpdateCallback(
159163
meta: R3HmrMetadata,
160164
): o.DeclareFunctionStmt {
161165
const namespaces = 'ɵɵnamespaces';
162-
const params = [meta.className, namespaces, ...meta.localDependencies].map(
163-
(name) => new o.FnParam(name, o.DYNAMIC_TYPE),
164-
);
166+
const params = [meta.className, namespaces].map((name) => new o.FnParam(name, o.DYNAMIC_TYPE));
165167
const body: o.Statement[] = [];
166168

169+
for (const local of meta.localDependencies) {
170+
params.push(new o.FnParam(local.name));
171+
}
172+
167173
// Declare variables that read out the individual namespaces.
168174
for (let i = 0; i < meta.namespaceDependencies.length; i++) {
169175
body.push(

0 commit comments

Comments
 (0)