fix(core): replace metadata in place during HMR (#59644)

crisbeto · AndrewKushnir · commit d7575c201cfd · 2025-01-21T09:50:27.000-08:00
Currently during HMR we swap out the entire module definition (e.g. `MyComp.ɵcmp = newDef`). In standalone components and most module-based ones this works fine, however in some cases (e.g. circular dependencies) the compiler can produce a `setComponentScope` call for a module-based component. This call doesn't make it into the HMR replacement function, because it is defined in the module's file, not the component's. As a result, the dependencies of these components are cleared out upon replacement. A secondary problem is that the `directiveDefs` and `pipeDefs` fields can save references to definitions that later become stale as a result of HMR. These changes resolve both issues by: 1. Performing the replacement by copying the properties from the new definition onto the old one, while keeping it in place. 2. Preserving the initial `directiveDefs`, `pipeDefs` and `setInput`. Fixes #59639. PR Close #59644
diff --git a/packages/core/src/render3/hmr.ts b/packages/core/src/render3/hmr.ts
@@ -7,7 +7,7 @@
  */
 
 import {Type} from '../interface/type';
-import {assertDefined, assertNotEqual} from '../util/assert';
+import {assertDefined, assertEqual, assertNotEqual} from '../util/assert';
 import {assertLView} from './assert';
 import {getComponentDef} from './def_getters';
 import {assertComponentDef} from './errors';
@@ -45,6 +45,7 @@ import {destroyLView, removeViewFromDOM} from './node_manipulation';
 import {RendererFactory} from './interfaces/renderer';
 import {NgZone} from '../zone';
 import {ViewEncapsulation} from '../metadata/view';
+import {NG_COMP_DEF} from './fields';
 
 /**
  * Replaces the metadata of a component type and re-renders all live instances of the component.
@@ -61,7 +62,7 @@ export function ɵɵreplaceMetadata(
   locals: unknown[],
 ) {
   ngDevMode && assertComponentDef(type);
-  const oldDef = getComponentDef(type)!;
+  const currentDef = getComponentDef(type)!;
 
   // The reason `applyMetadata` is a callback that is invoked (almost) immediately is because
   // the compiler usually produces more code than just the component definition, e.g. there
@@ -70,6 +71,13 @@ export function ɵɵreplaceMetadata(
   // them at the right time.
   applyMetadata.apply(null, [type, namespaces, ...locals]);
 
+  const {newDef, oldDef} = mergeWithExistingDefinition(currentDef, getComponentDef(type)!);
+
+  // TODO(crisbeto): the `applyMetadata` call above will replace the definition on the type.
+  // Ideally we should adjust the compiler output so the metadata is returned, however that'll
+  // require some internal changes. We re-add the metadata here manually.
+  (type as any)[NG_COMP_DEF] = newDef;
+
   // If a `tView` hasn't been created yet, it means that this component hasn't been instantianted
   // before. In this case there's nothing left for us to do aside from patching it in.
   if (oldDef.tView) {
@@ -78,18 +86,54 @@ export function ɵɵreplaceMetadata(
       // Note: we have the additional check, because `IsRoot` can also indicate
       // a component created through something like `createComponent`.
       if (root[FLAGS] & LViewFlags.IsRoot && root[PARENT] === null) {
-        recreateMatchingLViews(oldDef, root);
+        recreateMatchingLViews(newDef, oldDef, root);
       }
     }
   }
 }
 
+/**
+ * Merges two component definitions while preseving the original one in place.
+ * @param currentDef Definition that should receive the new metadata.
+ * @param newDef Source of the new metadata.
+ */
+function mergeWithExistingDefinition(
+  currentDef: ComponentDef<unknown>,
+  newDef: ComponentDef<unknown>,
+) {
+  // Clone the current definition since we reference its original data further
+  // down in the replacement process (e.g. when destroying the renderer).
+  const clone = {...currentDef};
+
+  // Assign the new metadata in place while preserving the object literal. It's important to
+  // Keep the object in place, because there can be references to it, for example in the
+  // `directiveDefs` of another definition.
+  const replacement = Object.assign(currentDef, newDef, {
+    // We need to keep the existing directive and pipe defs, because they can get patched on
+    // by a call to `setComponentScope` from a module file. That call won't make it into the
+    // HMR replacement function, because it lives in an entirely different file.
+    directiveDefs: clone.directiveDefs,
+    pipeDefs: clone.pipeDefs,
+
+    // Preserve the old `setInput` function, because it has some state.
+    // This is fine, because the component instance is preserved as well.
+    setInput: clone.setInput,
+  });
+
+  ngDevMode && assertEqual(replacement, currentDef, 'Expected definition to be merged in place');
+  return {newDef: replacement, oldDef: clone};
+}
+
 /**
  * Finds all LViews matching a specific component definition and recreates them.
  * @param oldDef Component definition to search for.
  * @param rootLView View from which to start the search.
  */
-function recreateMatchingLViews(oldDef: ComponentDef<unknown>, rootLView: LView): void {
+function recreateMatchingLViews(
+  newDef: ComponentDef<unknown>,
+  oldDef: ComponentDef<unknown>,
+  rootLView: LView,
+): void {
   ngDevMode &&
     assertDefined(
       oldDef.tView,
@@ -102,7 +146,7 @@ function recreateMatchingLViews(oldDef: ComponentDef<unknown>, rootLView: LView)
   // produce false positives when using inheritance.
   if (tView === oldDef.tView) {
     ngDevMode && assertComponentDef(oldDef.type);
-    recreateLView(getComponentDef(oldDef.type)!, oldDef, rootLView);
+    recreateLView(newDef, oldDef, rootLView);
     return;
   }
 
@@ -112,14 +156,14 @@ function recreateMatchingLViews(oldDef: ComponentDef<unknown>, rootLView: LView)
     if (isLContainer(current)) {
       // The host can be an LView if a component is injecting `ViewContainerRef`.
       if (isLView(current[HOST])) {
-        recreateMatchingLViews(oldDef, current[HOST]);
+        recreateMatchingLViews(newDef, oldDef, current[HOST]);
       }
 
       for (let j = CONTAINER_HEADER_OFFSET; j < current.length; j++) {
-        recreateMatchingLViews(oldDef, current[j]);
+        recreateMatchingLViews(newDef, oldDef, current[j]);
       }
     } else if (isLView(current)) {
-      recreateMatchingLViews(oldDef, current);
+      recreateMatchingLViews(newDef, oldDef, current);
     }
   }
 }
diff --git a/packages/core/test/acceptance/hmr_spec.ts b/packages/core/test/acceptance/hmr_spec.ts
@@ -29,13 +29,15 @@ import {
   ViewEncapsulation,
   ɵNG_COMP_DEF,
   ɵɵreplaceMetadata,
+  ɵɵsetComponentScope,
 } from '@angular/core';
 import {TestBed} from '@angular/core/testing';
 import {compileComponent} from '@angular/core/src/render3/jit/directive';
 import {angularCoreEnv} from '@angular/core/src/render3/jit/environment';
 import {clearTranslations, loadTranslations} from '@angular/localize';
 import {computeMsgId} from '@angular/compiler';
 import {EVENT_MANAGER_PLUGINS} from '@angular/platform-browser';
+import {ComponentType} from '@angular/core/src/render3';
 
 describe('hot module replacement', () => {
   it('should recreate a single usage of a basic component', () => {
@@ -529,6 +531,36 @@ describe('hot module replacement', () => {
     );
   });
 
+  it('should carry over dependencies defined by setComponentScope', () => {
+    // In some cases the AoT compiler produces a `setComponentScope` for non-standalone
+    // components. We simulate it here by declaring two components that are not standalone
+    // and manually calling `setComponentScope`.
+    @Component({selector: 'child-cmp', template: 'hello', standalone: false})
+    class ChildCmp {}
+
+    @Component({template: 'Initial <child-cmp/>', standalone: false})
+    class RootCmp {}
+
+    ɵɵsetComponentScope(RootCmp as ComponentType<RootCmp>, [ChildCmp], []);
+
+    const fixture = TestBed.createComponent(RootCmp);
+    fixture.detectChanges();
+    markNodesAsCreatedInitially(fixture.nativeElement);
+    expectHTML(fixture.nativeElement, 'Initial <child-cmp>hello</child-cmp>');
+
+    replaceMetadata(RootCmp, {
+      standalone: false,
+      template: 'Changed <child-cmp/>',
+    });
+    fixture.detectChanges();
+
+    const recreatedNodes = childrenOf(fixture.nativeElement);
+    verifyNodesRemainUntouched(fixture.nativeElement, recreatedNodes);
+    verifyNodesWereRecreated(recreatedNodes);
+
+    expectHTML(fixture.nativeElement, 'Changed <child-cmp>hello</child-cmp>');
+  });
+
   describe('queries', () => {
     it('should update ViewChildren query results', async () => {
       @Component({
