From ce3b6641fbbbc968ee2ddf037dbc5ea70b8f1b07 Mon Sep 17 00:00:00 2001
From: Kristiyan Kostadinov <crisbeto@abv.bg>
Date: Sat, 28 Dec 2024 10:01:37 +0200
Subject: [PATCH] fix(compiler-cli): account for more expression types when
 determining HMR dependencies (#59323)

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 https://github.com/angular/angular/issues/59310#issuecomment-2563963501.

PR Close #59323
---
 .../src/ngtsc/hmr/src/extract_dependencies.ts | 44 +++++++++++++++++--
 packages/compiler-cli/test/ngtsc/hmr_spec.ts  | 27 ++++++++++++
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/packages/compiler-cli/src/ngtsc/hmr/src/extract_dependencies.ts b/packages/compiler-cli/src/ngtsc/hmr/src/extract_dependencies.ts
index fa0d444bfbe..cd02ff91e30 100644
--- 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
index 537b328e81c..51d6047485e 100644
--- 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) {',
+      );
+    });
   });
 });