fix(compiler-cli): account for more expression types when determining HMR dependencies (#59323)

crisbeto · thePunderWoman · commit ce3b6641fbbb · 2025-01-06T17:25:57.000Z
During the HMR dependency analysis we need to check if an identifier is top-level or not. We do this by looking at each identifier and its parent, however we didn't account for some cases. These changes expand our logic to cover more of the common node types. Related to #59310 (comment). PR Close #59323
diff --git a/packages/compiler-cli/src/ngtsc/hmr/src/extract_dependencies.ts b/packages/compiler-cli/src/ngtsc/hmr/src/extract_dependencies.ts
@@ -209,8 +209,24 @@ class PotentialTopLevelReadsVisitor extends o.RecursiveAstVisitor {
       return parent.expression === node || parent.arguments.includes(node);
     }
 
-    // Identifier used in a property read is only top-level if it's the expression.
-    if (ts.isPropertyAccessExpression(parent)) {
+    // Identifier used in a nested expression is only top-level if it's the actual expression.
+    if (
+      ts.isPropertyAccessExpression(parent) ||
+      ts.isComputedPropertyName(parent) ||
+      ts.isTemplateSpan(parent) ||
+      ts.isSpreadAssignment(parent) ||
+      ts.isSpreadElement(parent) ||
+      ts.isAwaitExpression(parent) ||
+      ts.isNonNullExpression(parent) ||
+      ts.isIfStatement(parent) ||
+      ts.isDoStatement(parent) ||
+      ts.isWhileStatement(parent) ||
+      ts.isForInStatement(parent) ||
+      ts.isForOfStatement(parent) ||
+      ts.isSwitchStatement(parent) ||
+      ts.isCaseClause(parent) ||
+      ts.isThrowStatement(parent)
+    ) {
       return parent.expression === node;
     }
 
@@ -224,11 +240,31 @@ class PotentialTopLevelReadsVisitor extends o.RecursiveAstVisitor {
       return parent.initializer === node;
     }
 
-    // Identifier in a class is only top level if it's the name.
-    if (ts.isClassDeclaration(parent)) {
+    // Identifier in a declaration is only top level if it's the name.
+    // In shorthand assignments the name is also the value.
+    if (
+      ts.isClassDeclaration(parent) ||
+      ts.isFunctionDeclaration(parent) ||
+      ts.isVariableDeclaration(parent) ||
+      ts.isShorthandPropertyAssignment(parent)
+    ) {
       return parent.name === node;
     }
 
+    if (ts.isElementAccessExpression(parent)) {
+      return parent.expression === node || parent.argumentExpression === node;
+    }
+
+    if (ts.isBinaryExpression(parent)) {
+      return parent.left === node || parent.right === node;
+    }
+
+    // It's unlikely that we'll run into imports/exports in this use case.
+    // We handle them since it's simple and for completeness' sake.
+    if (ts.isImportSpecifier(parent) || ts.isExportSpecifier(parent)) {
+      return (parent.propertyName || parent.name) === node;
+    }
+
     // Otherwise it's not top-level.
     return false;
   }
diff --git a/packages/compiler-cli/test/ngtsc/hmr_spec.ts b/packages/compiler-cli/test/ngtsc/hmr_spec.ts
@@ -349,5 +349,32 @@ runInEachFileSystem(() => {
       const hmrContents = env.driveHmr('test.ts', 'Foo');
       expect(hmrContents).toBe(null);
     });
+
+    it('should capture shorthand property assignment dependencies', () => {
+      enableHmr();
+      env.write(
+        'test.ts',
+        `
+          import {Component} from '@angular/core';
+
+          const providers: any[] = [];
+
+          @Component({template: '', providers})
+          export class Cmp {}
+        `,
+      );
+
+      env.driveMain();
+
+      const jsContents = env.getContents('test.js');
+      const hmrContents = env.driveHmr('test.ts', 'Cmp');
+
+      expect(jsContents).toContain(
+        'ɵɵreplaceMetadata(Cmp, m.default, [i0], [providers, Component]));',
+      );
+      expect(hmrContents).toContain(
+        'export default function Cmp_UpdateMetadata(Cmp, ɵɵnamespaces, providers, Component) {',
+      );
+    });
   });
 });
