Skip to content

Commit

Permalink
refactor(core): replace usages of removeChild (#57203)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
crisbeto authored and thePunderWoman committed Aug 7, 2024
1 parent d465061 commit 513a4fe
Show file tree
Hide file tree
Showing 32 changed files with 53 additions and 95 deletions.
11 changes: 2 additions & 9 deletions packages/animations/browser/src/render/animation_renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
}

Expand Down
8 changes: 7 additions & 1 deletion packages/animations/browser/src/render/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion packages/animations/browser/test/dsl/animation_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ describe('Animation', () => {
});

afterEach(() => {
document.body.removeChild(rootElement);
rootElement.remove();
});

describe('validation', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('AnimationTrigger', () => {
});

afterEach(() => {
document.body.removeChild(element);
element.remove();
});

describe('trigger validation', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const DEFAULT_NAMESPACE_ID = 'id';
});

afterEach(() => {
document.body.removeChild(element);
element.remove();
});

function makeEngine(normalizer?: AnimationStyleNormalizer) {
Expand Down
4 changes: 1 addition & 3 deletions packages/common/http/src/jsonp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
4 changes: 4 additions & 0 deletions packages/common/http/test/jsonp_mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ export class MockScriptElement {
removeEventListener(event: 'load' | 'error'): void {
delete this.listeners[event];
}

remove() {
this.ownerDocument.removeNode(this);
}
}

export class MockDocument {
Expand Down
8 changes: 4 additions & 4 deletions packages/common/test/viewport_scroller_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,8 @@ describe('BrowserViewportScroller', () => {
return {
anchorNode,
cleanup: () => {
document.body.removeChild(tallItem);
document.body.removeChild(anchorNode);
tallItem.remove();
anchorNode.remove();
},
};
}
Expand All @@ -128,8 +128,8 @@ describe('BrowserViewportScroller', () => {
return {
anchorNode,
cleanup: () => {
document.body.removeChild(tallItem);
document.body.removeChild(elementWithShadowRoot);
tallItem.remove();
elementWithShadowRoot.remove();
},
};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/render3/interfaces/renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 1 addition & 7 deletions packages/core/src/render3/interfaces/renderer_dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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;
}

Expand Down
20 changes: 1 addition & 19 deletions packages/core/src/render3/node_manipulation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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);
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/sanitization/html_sanitizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/sanitization/inert_body.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/acceptance/renderer_factory_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
4 changes: 1 addition & 3 deletions packages/core/test/acceptance/view_container_ref_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2621,9 +2621,7 @@ describe('ViewContainerRef', () => {
containerEl!.appendChild(rootEl);
},
removeAllRootElements() {
if (containerEl) {
containerEl.parentNode?.removeChild(containerEl);
}
containerEl?.remove();
},
};
}
Expand Down
3 changes: 0 additions & 3 deletions packages/core/test/bundling/defer/bundle.golden_symbols.json
Original file line number Diff line number Diff line change
Expand Up @@ -2285,9 +2285,6 @@
{
"name": "nativeInsertBefore"
},
{
"name": "nativeParentNode"
},
{
"name": "nextNgElementId"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1166,9 +1166,6 @@
{
"name": "nativeInsertBefore"
},
{
"name": "nativeParentNode"
},
{
"name": "nativeRemoveNode"
},
Expand Down
4 changes: 2 additions & 2 deletions packages/core/test/dom/dom_adapter_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/');
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
5 changes: 1 addition & 4 deletions packages/core/test/transfer_state_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion packages/elements/test/create-custom-element-env_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('createCustomElement with env injector', () => {
});

afterEach(() => {
document.body.removeChild(testContainer);
testContainer.remove();
(testContainer as any) = null;
});

Expand Down
2 changes: 1 addition & 1 deletion packages/elements/test/create-custom-element_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ describe('createCustomElement', () => {

afterAll(() => {
destroyPlatform();
document.body.removeChild(testContainer);
testContainer.remove();
(testContainer as any) = null;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
});

Expand Down
4 changes: 1 addition & 3 deletions packages/platform-browser/src/browser/browser_adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
10 changes: 4 additions & 6 deletions packages/platform-browser/src/dom/dom_renderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)));
Expand Down
9 changes: 4 additions & 5 deletions packages/platform-browser/test/dom/dom_renderer_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down
3 changes: 1 addition & 2 deletions packages/platform-server/test/hydration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion packages/upgrade/src/common/src/upgrade_helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
Loading

0 comments on commit 513a4fe

Please sign in to comment.