fix(core): run HMR replacement in the zone (#59562)

crisbeto · pkozlowski-opensource · commit 357795cb96a1 · 2025-01-16T12:36:58.000+01:00
Fixes that in some cases the HMR replacement function was being run outside the zone which meant that change detection would break down after a replacement. Fixes #59559. PR Close #59562
diff --git a/packages/core/src/render3/hmr.ts b/packages/core/src/render3/hmr.ts
@@ -31,6 +31,7 @@ import {
   FLAGS,
   HEADER_OFFSET,
   HOST,
+  INJECTOR,
   LView,
   LViewFlags,
   NEXT,
@@ -41,6 +42,7 @@ import {
 import {assertTNodeType} from './node_assert';
 import {destroyLView, removeViewFromDOM} from './node_manipulation';
 import {RendererFactory} from './interfaces/renderer';
+import {NgZone} from '../zone';
 
 /**
  * Replaces the metadata of a component type and re-renders all live instances of the component.
@@ -149,51 +151,60 @@ function recreateLView(
   const tNode = lView[T_HOST] as TElementNode;
   ngDevMode && assertTNodeType(tNode, TNodeType.Element);
   ngDevMode && assertNotEqual(newDef, oldDef, 'Expected different component definition');
+  const zone = lView[INJECTOR].get(NgZone, null);
+  const recreate = () => {
+    // Recreate the TView since the template might've changed.
+    const newTView = getOrCreateComponentTView(newDef);
 
-  // Recreate the TView since the template might've changed.
-  const newTView = getOrCreateComponentTView(newDef);
+    // Always force the creation of a new renderer to ensure state captured during construction
+    // stays consistent with the new component definition by clearing any old cached factories.
+    const rendererFactory = lView[ENVIRONMENT].rendererFactory;
+    clearRendererCache(rendererFactory, oldDef);
 
-  // Always force the creation of a new renderer to ensure state captured during construction
-  // stays consistent with the new component definition by clearing any old cached factories.
-  const rendererFactory = lView[ENVIRONMENT].rendererFactory;
-  clearRendererCache(rendererFactory, oldDef);
+    // Create a new LView from the new TView, but reusing the existing TNode and DOM node.
+    const newLView = createLView(
+      parentLView,
+      newTView,
+      instance,
+      getInitialLViewFlagsFromDef(newDef),
+      host,
+      tNode,
+      null,
+      rendererFactory.createRenderer(host, newDef),
+      null,
+      null,
+      null,
+    );
 
-  // Create a new LView from the new TView, but reusing the existing TNode and DOM node.
-  const newLView = createLView(
-    parentLView,
-    newTView,
-    instance,
-    getInitialLViewFlagsFromDef(newDef),
-    host,
-    tNode,
-    null,
-    rendererFactory.createRenderer(host, newDef),
-    null,
-    null,
-    null,
-  );
+    // Detach the LView from its current place in the tree so we don't
+    // start traversing any siblings and modifying their structure.
+    replaceLViewInTree(parentLView, lView, newLView, tNode.index);
 
-  // Detach the LView from its current place in the tree so we don't
-  // start traversing any siblings and modifying their structure.
-  replaceLViewInTree(parentLView, lView, newLView, tNode.index);
+    // Destroy the detached LView.
+    destroyLView(lView[TVIEW], lView);
 
-  // Destroy the detached LView.
-  destroyLView(lView[TVIEW], lView);
+    // Remove the nodes associated with the destroyed LView. This removes the
+    // descendants, but not the host which we want to stay in place.
+    removeViewFromDOM(lView[TVIEW], lView);
 
-  // Remove the nodes associated with the destroyed LView. This removes the
-  // descendants, but not the host which we want to stay in place.
-  removeViewFromDOM(lView[TVIEW], lView);
+    // Reset the content projection state of the TNode before the first render.
+    // Note that this has to happen after the LView has been destroyed or we
+    // risk some projected nodes not being removed correctly.
+    resetProjectionState(tNode);
 
-  // Reset the content projection state of the TNode before the first render.
-  // Note that this has to happen after the LView has been destroyed or we
-  // risk some projected nodes not being removed correctly.
-  resetProjectionState(tNode);
+    // Creation pass for the new view.
+    renderView(newTView, newLView, instance);
 
-  // Creation pass for the new view.
-  renderView(newTView, newLView, instance);
+    // Update pass for the new view.
+    refreshView(newTView, newLView, newTView.template, instance);
+  };
 
-  // Update pass for the new view.
-  refreshView(newTView, newLView, newTView.template, instance);
+  // The callback isn't guaranteed to be inside the Zone so we need to bring it in ourselves.
+  if (zone === null) {
+    recreate();
+  } else {
+    zone.run(recreate);
+  }
 }
 
 /**
diff --git a/packages/core/test/acceptance/hmr_spec.ts b/packages/core/test/acceptance/hmr_spec.ts
@@ -15,6 +15,7 @@ import {
   inject,
   InjectionToken,
   Input,
+  NgZone,
   OnChanges,
   OnDestroy,
   OnInit,
@@ -32,6 +33,7 @@ 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';
 
 describe('hot module replacement', () => {
   it('should recreate a single usage of a basic component', () => {
@@ -1213,6 +1215,47 @@ describe('hot module replacement', () => {
       fixture.detectChanges();
       expect(count).toBe(2);
     });
+
+    it('should bind events inside the NgZone after a replacement', () => {
+      const calls: {name: string; inZone: boolean}[] = [];
+
+      @Component({template: `<button (click)="clicked()"></button>`})
+      class App {
+        clicked() {}
+      }
+
+      TestBed.configureTestingModule({
+        providers: [
+          {
+            // Note: TestBed brings things into the zone even if they aren't which makes this
+            // test hard to write. We have to intercept the listener being bound at the renderer
+            // level in order to get a true sense if it'll be bound inside or outside the zone.
+            // We do so with a custom event manager.
+            provide: EVENT_MANAGER_PLUGINS,
+            multi: true,
+            useValue: {
+              supports: () => true,
+              addEventListener: (_: unknown, name: string) => {
+                calls.push({name, inZone: NgZone.isInAngularZone()});
+                return () => {};
+              },
+            },
+          },
+        ],
+      });
+
+      const fixture = TestBed.createComponent(App);
+      fixture.detectChanges();
+      expect(calls).toEqual([{name: 'click', inZone: true}]);
+
+      replaceMetadata(App, {template: '<button class="foo" (click)="clicked()"></button>'});
+      fixture.detectChanges();
+
+      expect(calls).toEqual([
+        {name: 'click', inZone: true},
+        {name: 'click', inZone: true},
+      ]);
+    });
   });
 
   describe('directives', () => {
