fix(core): animation sometimes renderer not being destroyed during HMR (#59574)

crisbeto · AndrewKushnir · commit a4eb74c79cca · 2025-01-16T11:36:43.000-08:00
These changes aim to resolve the issue that prompted #59514. The animations module is a bit tricky for HMR, because it schedules the destruction of its renderer after the currently-running animations are done. If there are no running animations, the renderer gets destroyed next time around. This is a problem, because it means that the styles can stay around for a long time. These changes resolve the issue by: 1. Moving the cleanup of the renderer to after the destruction of the old view. This ensures that the usual clean up flow has been kicked off. 2. Flushing the animations when a component is replaced to ensure that the renderer is cleaned up in a timely manner. PR Close #59574
diff --git a/packages/animations/browser/src/render/animation_renderer.ts b/packages/animations/browser/src/render/animation_renderer.ts
@@ -138,6 +138,8 @@ export class AnimationRendererFactory implements RendererFactory2 {
    * @param componentId ID of the component that is being replaced.
    */
   protected componentReplaced(componentId: string) {
+    // Flush the engine since the renderer destruction waits for animations to be done.
+    this.engine.flush();
     (this.delegate as {componentReplaced?: (id: string) => void}).componentReplaced?.(componentId);
   }
 }
diff --git a/packages/core/src/render3/hmr.ts b/packages/core/src/render3/hmr.ts
@@ -36,6 +36,7 @@ import {
   LViewFlags,
   NEXT,
   PARENT,
+  RENDERER,
   T_HOST,
   TVIEW,
 } from './interfaces/view';
@@ -156,11 +157,6 @@ function recreateLView(
     // 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);
-
     // Create a new LView from the new TView, but reusing the existing TNode and DOM node.
     const newLView = createLView(
       parentLView,
@@ -170,7 +166,7 @@ function recreateLView(
       host,
       tNode,
       null,
-      rendererFactory.createRenderer(host, newDef),
+      null, // The renderer will be created a bit further down once the old one is destroyed.
       null,
       null,
       null,
@@ -183,6 +179,15 @@ function recreateLView(
     // Destroy the detached LView.
     destroyLView(lView[TVIEW], lView);
 
+    // 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 ached factories.
+    const rendererFactory = lView[ENVIRONMENT].rendererFactory;
+    clearRendererCache(rendererFactory, oldDef);
+
+    // Patch a brand-new renderer onto the new view only after the old
+    // view is destroyed so that the runtime doesn't try to reuse it.
+    newLView[RENDERER] = rendererFactory.createRenderer(host, newDef);
+
     // 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);

Original file line number	Diff line number	Diff line change
`@@ -138,6 +138,8 @@ export class AnimationRendererFactory implements RendererFactory2 {`
`138`	`138`	`* @param componentId ID of the component that is being replaced.`
`139`	`139`	`*/`
`140`	`140`	`protected componentReplaced(componentId: string) {`
	`141`	`+ // Flush the engine since the renderer destruction waits for animations to be done.`
	`142`	`+ this.engine.flush();`
`141`	`143`	`(this.delegate as {componentReplaced?: (id: string) => void}).componentReplaced?.(componentId);`
`142`	`144`	`}`
`143`	`145`	`}`