fix(core): destroy renderer when replacing styles during HMR (#59514)

crisbeto · kirjs · commit a7f824a7ecb2 · 2025-01-14T11:22:07.000-05:00
Currently when we swap out the component during HMR, we remove the renderer from the cache, but we never destroy it which means that its styles are still in the DOM. This can cause the old styles to leak into the component after they're replaced. These changes add a `destroy` call to ensure that they're removed. PR Close #59514
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} from '../util/assert';
+import {assertDefined, assertNotEqual} from '../util/assert';
 import {assertLView} from './assert';
 import {getComponentDef} from './def_getters';
 import {assertComponentDef} from './errors';
@@ -82,23 +82,23 @@ export function ɵɵreplaceMetadata(
 
 /**
  * Finds all LViews matching a specific component definition and recreates them.
- * @param def Component definition to search for.
+ * @param oldDef Component definition to search for.
  * @param rootLView View from which to start the search.
  */
-function recreateMatchingLViews(def: ComponentDef<unknown>, rootLView: LView): void {
+function recreateMatchingLViews(oldDef: ComponentDef<unknown>, rootLView: LView): void {
   ngDevMode &&
     assertDefined(
-      def.tView,
+      oldDef.tView,
       'Expected a component definition that has been instantiated at least once',
     );
 
   const tView = rootLView[TVIEW];
 
   // Use `tView` to match the LView since `instanceof` can
   // produce false positives when using inheritance.
-  if (tView === def.tView) {
-    ngDevMode && assertComponentDef(def.type);
-    recreateLView(getComponentDef(def.type)!, rootLView);
+  if (tView === oldDef.tView) {
+    ngDevMode && assertComponentDef(oldDef.type);
+    recreateLView(getComponentDef(oldDef.type)!, oldDef, rootLView);
     return;
   }
 
@@ -107,10 +107,10 @@ function recreateMatchingLViews(def: ComponentDef<unknown>, rootLView: LView): v
 
     if (isLContainer(current)) {
       for (let i = CONTAINER_HEADER_OFFSET; i < current.length; i++) {
-        recreateMatchingLViews(def, current[i]);
+        recreateMatchingLViews(oldDef, current[i]);
       }
     } else if (isLView(current)) {
-      recreateMatchingLViews(def, current);
+      recreateMatchingLViews(oldDef, current);
     }
   }
 }
@@ -131,10 +131,15 @@ function clearRendererCache(factory: RendererFactory, def: ComponentDef<unknown>
 
 /**
  * Recreates an LView in-place from a new component definition.
- * @param def Definition from which to recreate the view.
+ * @param newDef Definition from which to recreate the view.
+ * @param oldDef Previous component definition being swapped out.
  * @param lView View to be recreated.
  */
-function recreateLView(def: ComponentDef<unknown>, lView: LView<unknown>): void {
+function recreateLView(
+  newDef: ComponentDef<unknown>,
+  oldDef: ComponentDef<unknown>,
+  lView: LView<unknown>,
+): void {
   const instance = lView[CONTEXT];
   const host = lView[HOST]!;
   // In theory the parent can also be an LContainer, but it appears like that's
@@ -143,25 +148,26 @@ function recreateLView(def: ComponentDef<unknown>, lView: LView<unknown>): void
   ngDevMode && assertLView(parentLView);
   const tNode = lView[T_HOST] as TElementNode;
   ngDevMode && assertTNodeType(tNode, TNodeType.Element);
+  ngDevMode && assertNotEqual(newDef, oldDef, 'Expected different component definition');
 
   // Recreate the TView since the template might've changed.
-  const newTView = getOrCreateComponentTView(def);
+  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, def);
+  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(def),
+    getInitialLViewFlagsFromDef(newDef),
     host,
     tNode,
     null,
-    rendererFactory.createRenderer(host, def),
+    rendererFactory.createRenderer(host, newDef),
     null,
     null,
     null,
diff --git a/packages/platform-browser/src/dom/dom_renderer.ts b/packages/platform-browser/src/dom/dom_renderer.ts
@@ -201,6 +201,9 @@ export class DomRendererFactory2 implements RendererFactory2, OnDestroy {
    * @param componentId ID of the component that is being replaced.
    */
   protected componentReplaced(componentId: string) {
+    // Destroy the renderer so the styles get removed from the DOM, otherwise
+    // they may leak back into the component together with the new ones.
+    this.rendererByCompId.get(componentId)?.destroy();
     this.rendererByCompId.delete(componentId);
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -201,6 +201,9 @@ export class DomRendererFactory2 implements RendererFactory2, OnDestroy {`
`201`	`201`	`* @param componentId ID of the component that is being replaced.`
`202`	`202`	`*/`
`203`	`203`	`protected componentReplaced(componentId: string) {`
	`204`	`+ // Destroy the renderer so the styles get removed from the DOM, otherwise`
	`205`	`+ // they may leak back into the component together with the new ones.`
	`206`	`+ this.rendererByCompId.get(componentId)?.destroy();`
`204`	`207`	`this.rendererByCompId.delete(componentId);`
`205`	`208`	`}`
`206`	`209`	`}`