refactor(core): replace usages of removeChild (#57203)

crisbeto · thePunderWoman · commit e36647310958 · 2024-08-07T16:46:09.000Z
These changes replace most usages of `removeChild` with `remove`. The latter has the advantage of not having to look up the `parentNode` and ensure that the child being removed actually belongs to the specific parent. The refactor should be fairly safe since all the browsers we cover support `remove`. [Something similar was done in Components](angular/components#23592) some time ago and there haven't been any bug reports as a result. PR Close #57203
diff --git a/packages/animations/browser/src/render/animation_renderer.ts b/packages/animations/browser/src/render/animation_renderer.ts
@@ -29,15 +29,8 @@ export class AnimationRendererFactory implements RendererFactory2 {
     private engine: AnimationEngine,
     private _zone: NgZone,
   ) {
-    engine.onRemovalComplete = (element: any, delegate: Renderer2) => {
-      // Note: if a component element has a leave animation, and a host leave animation,
-      // the view engine will call `removeChild` for the parent
-      // component renderer as well as for the child component renderer.
-      // Therefore, we need to check if we already removed the element.
-      const parentNode = delegate?.parentNode(element);
-      if (parentNode) {
-        delegate.removeChild(parentNode, element);
-      }
+    engine.onRemovalComplete = (element: any, delegate: Renderer2 | null) => {
+      delegate?.removeChild(null, element);
     };
   }
 
diff --git a/packages/animations/browser/src/render/renderer.ts b/packages/animations/browser/src/render/renderer.ts
@@ -78,7 +78,13 @@ export class BaseAnimationRenderer implements Renderer2 {
   }
 
   removeChild(parent: any, oldChild: any, isHostElement?: boolean): void {
-    this.engine.onRemove(this.namespaceId, oldChild, this.delegate);
+    // Prior to the changes in #57203, this method wasn't being called at all by `core` if the child
+    // doesn't have a parent. There appears to be some animation-specific downstream logic that
+    // depends on the null check happening before the animation engine. This check keeps the old
+    // behavior while allowing `core` to not have to check for the parent element anymore.
+    if (this.parentNode(oldChild)) {
+      this.engine.onRemove(this.namespaceId, oldChild, this.delegate);
+    }
   }
 
   selectRootElement(selectorOrNode: any, preserveContent?: boolean) {
diff --git a/packages/animations/browser/test/dsl/animation_spec.ts b/packages/animations/browser/test/dsl/animation_spec.ts
@@ -56,7 +56,7 @@ describe('Animation', () => {
   });
 
   afterEach(() => {
-    document.body.removeChild(rootElement);
+    rootElement.remove();
   });
 
   describe('validation', () => {
diff --git a/packages/animations/browser/test/dsl/animation_trigger_spec.ts b/packages/animations/browser/test/dsl/animation_trigger_spec.ts
@@ -29,7 +29,7 @@ describe('AnimationTrigger', () => {
   });
 
   afterEach(() => {
-    document.body.removeChild(element);
+    element.remove();
   });
 
   describe('trigger validation', () => {
diff --git a/packages/animations/browser/test/render/timeline_animation_engine_spec.ts b/packages/animations/browser/test/render/timeline_animation_engine_spec.ts
@@ -39,7 +39,7 @@ import {MockAnimationDriver, MockAnimationPlayer} from '../../testing/src/mock_a
       document.body.appendChild(element);
     });
 
-    afterEach(() => document.body.removeChild(element));
+    afterEach(() => element.remove());
 
     it('should animate a timeline', () => {
       const engine = makeEngine(getBodyNode());
diff --git a/packages/animations/browser/test/render/transition_animation_engine_spec.ts b/packages/animations/browser/test/render/transition_animation_engine_spec.ts
@@ -49,7 +49,7 @@ const DEFAULT_NAMESPACE_ID = 'id';
     });
 
     afterEach(() => {
-      document.body.removeChild(element);
+      element.remove();
     });
 
     function makeEngine(normalizer?: AnimationStyleNormalizer) {
diff --git a/packages/common/http/src/jsonp.ts b/packages/common/http/src/jsonp.ts
@@ -164,9 +164,7 @@ export class JsonpClientBackend implements HttpBackend {
       // success, error, and cancellation paths, so it's extracted out for convenience.
       const cleanup = () => {
         // Remove the <script> tag if it's still on the page.
-        if (node.parentNode) {
-          node.parentNode.removeChild(node);
-        }
+        node.remove();
 
         // Remove the response callback from the callbackMap (window object in the
         // browser).
diff --git a/packages/common/http/test/jsonp_mock.ts b/packages/common/http/test/jsonp_mock.ts
@@ -21,6 +21,10 @@ export class MockScriptElement {
   removeEventListener(event: 'load' | 'error'): void {
     delete this.listeners[event];
   }
+
+  remove() {
+    this.ownerDocument.removeNode(this);
+  }
 }
 
 export class MockDocument {
diff --git a/packages/common/test/viewport_scroller_spec.ts b/packages/common/test/viewport_scroller_spec.ts
@@ -108,8 +108,8 @@ describe('BrowserViewportScroller', () => {
       return {
         anchorNode,
         cleanup: () => {
-          document.body.removeChild(tallItem);
-          document.body.removeChild(anchorNode);
+          tallItem.remove();
+          anchorNode.remove();
         },
       };
     }
@@ -128,8 +128,8 @@ describe('BrowserViewportScroller', () => {
       return {
         anchorNode,
         cleanup: () => {
-          document.body.removeChild(tallItem);
-          document.body.removeChild(elementWithShadowRoot);
+          tallItem.remove();
+          elementWithShadowRoot.remove();
         },
       };
     }
diff --git a/packages/core/src/render3/interfaces/renderer.ts b/packages/core/src/render3/interfaces/renderer.ts
@@ -43,7 +43,7 @@ export interface Renderer {
   destroyNode?: ((node: RNode) => void) | null;
   appendChild(parent: RElement, newChild: RNode): void;
   insertBefore(parent: RNode, newChild: RNode, refChild: RNode | null, isMove?: boolean): void;
-  removeChild(parent: RElement, oldChild: RNode, isHostElement?: boolean): void;
+  removeChild(parent: RElement | null, oldChild: RNode, isHostElement?: boolean): void;
   selectRootElement(selectorOrNode: string | any, preserveContent?: boolean): RElement;
 
   parentNode(node: RNode): RElement | null;
diff --git a/packages/core/src/render3/interfaces/renderer_dom.ts b/packages/core/src/render3/interfaces/renderer_dom.ts
@@ -34,12 +34,6 @@ export interface RNode {
    */
   nextSibling: RNode | null;
 
-  /**
-   * Removes a child from the current node and returns the removed node
-   * @param oldChild the child node to remove
-   */
-  removeChild(oldChild: RNode): RNode;
-
   /**
    * Insert a child node.
    *
@@ -77,7 +71,7 @@ export interface RElement extends RNode {
   ): void;
   addEventListener(type: string, listener: EventListener, useCapture?: boolean): void;
   removeEventListener(type: string, listener?: EventListener, options?: boolean): void;
-
+  remove(): void;
   setProperty?(name: string, value: any): void;
 }
 
diff --git a/packages/core/src/render3/node_manipulation.ts b/packages/core/src/render3/node_manipulation.ts
@@ -711,21 +711,6 @@ function nativeAppendOrInsertBefore(
   }
 }
 
-/** Removes a node from the DOM given its native parent. */
-function nativeRemoveChild(
-  renderer: Renderer,
-  parent: RElement,
-  child: RNode,
-  isHostElement?: boolean,
-): void {
-  renderer.removeChild(parent, child, isHostElement);
-}
-
-/** Checks if an element is a `<template>` node. */
-function isTemplateNode(node: RElement): node is RTemplate {
-  return node.tagName === 'TEMPLATE' && (node as RTemplate).content !== undefined;
-}
-
 /**
  * Returns a native parent of a given native node.
  */
@@ -951,10 +936,7 @@ export function getBeforeNodeForView(
  */
 export function nativeRemoveNode(renderer: Renderer, rNode: RNode, isHostElement?: boolean): void {
   ngDevMode && ngDevMode.rendererRemoveNode++;
-  const nativeParent = nativeParentNode(renderer, rNode);
-  if (nativeParent) {
-    nativeRemoveChild(renderer, nativeParent, rNode, isHostElement);
-  }
+  renderer.removeChild(null, rNode, isHostElement);
 }
 
 /**
diff --git a/packages/core/src/sanitization/html_sanitizer.ts b/packages/core/src/sanitization/html_sanitizer.ts
@@ -335,7 +335,7 @@ export function _sanitizeHtml(defaultDoc: any, unsafeHtmlInput: string): Trusted
     if (inertBodyElement) {
       const parent = getTemplateContent(inertBodyElement) || inertBodyElement;
       while (parent.firstChild) {
-        parent.removeChild(parent.firstChild);
+        parent.firstChild.remove();
       }
     }
   }
diff --git a/packages/core/src/sanitization/inert_body.ts b/packages/core/src/sanitization/inert_body.ts
@@ -51,7 +51,7 @@ class DOMParserHelper implements InertBodyHelper {
         // the `inertDocumentHelper` instead.
         return this.inertDocumentHelper.getInertBodyElement(html);
       }
-      body.removeChild(body.firstChild!);
+      body.firstChild?.remove();
       return body;
     } catch {
       return null;
diff --git a/packages/core/test/acceptance/renderer_factory_spec.ts b/packages/core/test/acceptance/renderer_factory_spec.ts
@@ -548,8 +548,8 @@ class MockRenderer implements Renderer2 {
   insertBefore(parent: Node, newChild: Node, refChild: Node | null): void {
     parent.insertBefore(newChild, refChild);
   }
-  removeChild(parent: RElement, oldChild: Node): void {
-    parent.removeChild(oldChild);
+  removeChild(parent: RElement, oldChild: Element): void {
+    oldChild.remove();
   }
   selectRootElement(selectorOrNode: string | any): RElement {
     return typeof selectorOrNode === 'string'
diff --git a/packages/core/test/acceptance/view_container_ref_spec.ts b/packages/core/test/acceptance/view_container_ref_spec.ts
@@ -2621,9 +2621,7 @@ describe('ViewContainerRef', () => {
           containerEl!.appendChild(rootEl);
         },
         removeAllRootElements() {
-          if (containerEl) {
-            containerEl.parentNode?.removeChild(containerEl);
-          }
+          containerEl?.remove();
         },
       };
     }
diff --git a/packages/core/test/bundling/defer/bundle.golden_symbols.json b/packages/core/test/bundling/defer/bundle.golden_symbols.json
@@ -2273,9 +2273,6 @@
   {
     "name": "nativeInsertBefore"
   },
-  {
-    "name": "nativeParentNode"
-  },
   {
     "name": "nextNgElementId"
   },
diff --git a/packages/core/test/bundling/hydration/bundle.golden_symbols.json b/packages/core/test/bundling/hydration/bundle.golden_symbols.json
@@ -1160,9 +1160,6 @@
   {
     "name": "nativeInsertBefore"
   },
-  {
-    "name": "nativeParentNode"
-  },
   {
     "name": "nativeRemoveNode"
   },
diff --git a/packages/core/test/dom/dom_adapter_spec.ts b/packages/core/test/dom/dom_adapter_spec.ts
@@ -49,7 +49,7 @@ describe('dom adapter', () => {
         headEl.appendChild(baseEl);
 
         const baseHref = getDOM().getBaseHref(defaultDoc);
-        headEl.removeChild(baseEl);
+        baseEl.remove();
         getDOM().resetBaseElement();
 
         expect(baseHref).toEqual('/drop/bass/connon/');
@@ -62,7 +62,7 @@ describe('dom adapter', () => {
         headEl.appendChild(baseEl);
 
         const baseHref = getDOM().getBaseHref(defaultDoc)!;
-        headEl.removeChild(baseEl);
+        baseEl.remove();
         getDOM().resetBaseElement();
 
         expect(baseHref.endsWith('/base')).toBe(true);
diff --git a/packages/core/test/render3/instructions/mock_renderer_factory.ts b/packages/core/test/render3/instructions/mock_renderer_factory.ts
@@ -41,8 +41,8 @@ class MockRenderer implements Renderer {
   insertBefore(parent: Node, newChild: Node, refChild: Node | null): void {
     parent.insertBefore(newChild, refChild);
   }
-  removeChild(parent: RElement, oldChild: Node): void {
-    parent.removeChild(oldChild);
+  removeChild(_parent: RElement | null, oldChild: RElement): void {
+    oldChild.remove();
   }
   selectRootElement(selectorOrNode: string | any): RElement {
     return typeof selectorOrNode === 'string'
diff --git a/packages/core/test/transfer_state_spec.ts b/packages/core/test/transfer_state_spec.ts
@@ -13,10 +13,7 @@ import {getDocument} from '../src/render3/interfaces/document';
 import {makeStateKey, TransferState} from '../src/transfer_state';
 
 function removeScriptTag(doc: Document, id: string) {
-  const existing = doc.getElementById(id);
-  if (existing) {
-    doc.body.removeChild(existing);
-  }
+  doc.getElementById(id)?.remove();
 }
 
 function addScriptTag(doc: Document, appId: string, data: object | string) {
diff --git a/packages/elements/test/create-custom-element-env_spec.ts b/packages/elements/test/create-custom-element-env_spec.ts
@@ -20,7 +20,7 @@ describe('createCustomElement with env injector', () => {
   });
 
   afterEach(() => {
-    document.body.removeChild(testContainer);
+    testContainer.remove();
     (testContainer as any) = null;
   });
 
diff --git a/packages/elements/test/create-custom-element_spec.ts b/packages/elements/test/create-custom-element_spec.ts
@@ -61,7 +61,7 @@ describe('createCustomElement', () => {
 
   afterAll(() => {
     destroyPlatform();
-    document.body.removeChild(testContainer);
+    testContainer.remove();
     (testContainer as any) = null;
   });
 
diff --git a/packages/platform-browser/animations/async/test/animation_renderer_spec.ts b/packages/platform-browser/animations/async/test/animation_renderer_spec.ts
@@ -122,9 +122,8 @@ type AnimationBrowserModule = typeof import('@angular/animations/browser');
     it("should hook into the engine's insert operations when removing children", async () => {
       const renderer = await makeRenderer();
       const engine = (renderer as any).delegate.engine as MockAnimationEngine;
-      const container = el('<div></div>');
 
-      renderer.removeChild(container, element, false);
+      renderer.removeChild(null, element, false);
       expect(engine.captures['onRemove'].pop()).toEqual([element]);
     });
 
diff --git a/packages/platform-browser/animations/test/animation_renderer_spec.ts b/packages/platform-browser/animations/test/animation_renderer_spec.ts
@@ -91,9 +91,8 @@ import {el} from '../../testing/src/browser_util';
     it("should hook into the engine's insert operations when removing children", () => {
       const renderer = makeRenderer();
       const engine = TestBed.inject(AnimationEngine) as MockAnimationEngine;
-      const container = el('<div></div>');
 
-      renderer.removeChild(container, element);
+      renderer.removeChild(null, element);
       expect(engine.captures['onRemove'].pop()).toEqual([element]);
     });
 
diff --git a/packages/platform-browser/src/browser/browser_adapter.ts b/packages/platform-browser/src/browser/browser_adapter.ts
@@ -35,9 +35,7 @@ export class BrowserDomAdapter extends GenericBrowserDomAdapter {
     el.dispatchEvent(evt);
   }
   override remove(node: Node): void {
-    if (node.parentNode) {
-      node.parentNode.removeChild(node);
-    }
+    (node as Element | Text | Comment).remove();
   }
   override createElement(tagName: string, doc?: Document): HTMLElement {
     doc = doc || this.getDefaultDocument();
diff --git a/packages/platform-browser/src/dom/dom_renderer.ts b/packages/platform-browser/src/dom/dom_renderer.ts
@@ -243,10 +243,8 @@ class DefaultDomRenderer2 implements Renderer2 {
     }
   }
 
-  removeChild(parent: any, oldChild: any): void {
-    if (parent) {
-      parent.removeChild(oldChild);
-    }
+  removeChild(_parent: any, oldChild: any): void {
+    oldChild.remove();
   }
 
   selectRootElement(selectorOrNode: string | any, preserveContent?: boolean): any {
@@ -448,8 +446,8 @@ class ShadowDomRenderer extends DefaultDomRenderer2 {
   override insertBefore(parent: any, newChild: any, refChild: any): void {
     return super.insertBefore(this.nodeOrShadowRoot(parent), newChild, refChild);
   }
-  override removeChild(parent: any, oldChild: any): void {
-    return super.removeChild(this.nodeOrShadowRoot(parent), oldChild);
+  override removeChild(_parent: any, oldChild: any): void {
+    return super.removeChild(null, oldChild);
   }
   override parentNode(node: any): any {
     return this.nodeOrShadowRoot(super.parentNode(this.nodeOrShadowRoot(node)));
diff --git a/packages/platform-browser/test/dom/dom_renderer_spec.ts b/packages/platform-browser/test/dom/dom_renderer_spec.ts
@@ -98,13 +98,12 @@ describe('DefaultDomRendererV2', () => {
   });
 
   describe('removeChild', () => {
-    it('should not error when removing a child with a different parent than given', () => {
-      const savedParent = document.createElement('div');
-      const realParent = document.createElement('div');
+    it('should not error when removing a child without passing a parent', () => {
+      const parent = document.createElement('div');
       const child = document.createElement('div');
 
-      realParent.appendChild(child);
-      renderer.removeChild(savedParent, child);
+      parent.appendChild(child);
+      renderer.removeChild(null, child);
     });
   });
 
diff --git a/packages/platform-browser/test/dom/events/event_manager_spec.ts b/packages/platform-browser/test/dom/events/event_manager_spec.ts
@@ -325,7 +325,7 @@ import {TestBed} from '@angular/core/testing';
       expect(timeoutId).not.toBe(null);
 
       // cleanup the DOM by removing the test element we attached earlier.
-      doc.body.removeChild(element);
+      element.remove();
       timeoutId && clearTimeout(timeoutId);
     });
 
diff --git a/packages/platform-server/test/hydration_spec.ts b/packages/platform-server/test/hydration_spec.ts
@@ -4102,8 +4102,7 @@ describe('platform-server hydration integration', () => {
           el = inject(ElementRef);
 
           ngAfterViewInit() {
-            const pTag = document.querySelector('p');
-            pTag?.parentElement?.removeChild(pTag);
+            document.querySelector('p')?.remove();
             const span = document.createElement('span');
             span.innerHTML = 'Appended span';
             this.el.nativeElement.appendChild(span);
diff --git a/packages/upgrade/src/common/src/upgrade_helper.ts b/packages/upgrade/src/common/src/upgrade_helper.ts
@@ -261,7 +261,7 @@ export class UpgradeHelper {
     let childNode: Node | null;
 
     while ((childNode = this.element.firstChild)) {
-      this.element.removeChild(childNode);
+      (childNode as Element | Comment | Text).remove();
       childNodes.push(childNode);
     }
 
diff --git a/packages/upgrade/src/common/test/helpers/common_test_helpers.ts b/packages/upgrade/src/common/test/helpers/common_test_helpers.ts

Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,13 @@ export class BaseAnimationRenderer implements Renderer2 {`
`78`	`78`	`}`
`79`	`79`
`80`	`80`	`removeChild(parent: any, oldChild: any, isHostElement?: boolean): void {`
`81`		`- this.engine.onRemove(this.namespaceId, oldChild, this.delegate);`
	`81`	+ // Prior to the changes in #57203, this method wasn't being called at all by `core` if the child
	`82`	`+ // doesn't have a parent. There appears to be some animation-specific downstream logic that`
	`83`	`+ // depends on the null check happening before the animation engine. This check keeps the old`
	`84`	+ // behavior while allowing `core` to not have to check for the parent element anymore.
	`85`	`+ if (this.parentNode(oldChild)) {`
	`86`	`+ this.engine.onRemove(this.namespaceId, oldChild, this.delegate);`
	`87`	`+ }`
`82`	`88`	`}`
`83`	`89`
`84`	`90`	`selectRootElement(selectorOrNode: any, preserveContent?: boolean) {`
Original file line number	Diff line number	Diff line change
`@@ -21,6 +21,10 @@ export class MockScriptElement {`
`21`	`21`	`removeEventListener(event: 'load' \| 'error'): void {`
`22`	`22`	`delete this.listeners[event];`
`23`	`23`	`}`
	`24`	`+`
	`25`	`+ remove() {`
	`26`	`+ this.ownerDocument.removeNode(this);`
	`27`	`+ }`
`24`	`28`	`}`
`25`	`29`
`26`	`30`	`export class MockDocument {`