Skip to content

Fix ChildNode.replaceWith throwing instead of replacing the node - #623

Merged
andrewiggins merged 1 commit into
Shopify:mainfrom
airhorns:polyfill-replace-with
Sep 3, 2026
Merged

andrewiggins merged 1 commit into
Shopify:mainfrom
airhorns:polyfill-replace-with

Conversation

@airhorns

Copy link
Copy Markdown
Contributor

Depends on #622 — the first of the two commits here is that PR. Review the second commit (Fix ChildNode.replaceWith…) only. Once #622 merges I'll rebase and this drops to a single commit. The dependency is real rather than cosmetic — see Why it stacks below.

The bug

ChildNode.replaceWith() passes its arguments to replaceChild() in the wrong order. The signature is replaceChild(newChild, oldChild), but it is called as:

const node = toNode(parent, nodes[0]);
const next = node[NEXT];             // NEXT of the *incoming* node, not of `this`
parent.replaceChild(this, node);     // args reversed: names `node` as the child to replace

replaceChild rejects an oldChild that isn't already a child of the parent. The incoming node is normally fresh with no parent, so every call on a node that has a parent throws:

Error: reference node is not a child of this parent

Two further consequences of the same mix-up:

  • next is read off the incoming (detached) node, so it is null, and the 2nd..nth arguments have no correct insertion point to anchor to.
  • replaceWith() with no arguments takes nodes[0] === undefined through toNode, which stringifies it — so instead of removing the node it tries to insert the literal text undefined.

Current behaviour on main, every case with a parent:

call result on main
el.replaceWith(fresh) (middle child) THREW
el.replaceWith(fresh) (last child) THREW
el.replaceWith(a, b) THREW
el.replaceWith('text') THREW
el.replaceWith() THREW
detached el.replaceWith(fresh) no-op ✅

Only the no-parent early return worked, so the method is entirely non-functional.

The fix

Remove this and insert the given nodes at its position in argument order:

const parent = this.parentNode;
if (!parent) return;
let next = this[NEXT];
while (next && nodes.includes(next)) next = next[NEXT];
parent.removeChild(this);
for (const node of nodes) {
  parent.insertBefore(toNode(parent, node), next);
}

This matches the spec: strings become text nodes, no arguments removes the node (same as remove()), and a node with no parent is left alone.

The while loop is the spec's viable next sibling. Anchoring on this[NEXT] alone breaks when a node is replaced with one of its own siblings — b.replaceWith(c) where c is b's next sibling would move c out of the parent and then try to insert before it, throwing again. Skipping anchors that are themselves being moved keeps b.replaceWith(c) and b.replaceWith(b) correct.

Why it stacks on #622

Replacing a middle child means inserting before a non-first child, which is exactly the sibling-chain bug #622 fixes. With this fix alone on main, the replacement lands in childNodes but never enters the forward sibling chain:

sibling chain : [ a-el, c-el ]           <- replacement missing
childNodes    : [ a-el, r-el, c-el ]     <- replacement present
querySelector : null

So replaceWith cannot be observably fixed without #622, and 4 of the tests below fail without it.

Tests

packages/polyfill/source/tests/ChildNode.test.ts — 12 tests covering single-node replacement at first/middle/last position, multiple nodes in argument order, strings, mixed nodes and strings, no arguments, a detached node, moving a node already in the tree, replacing a node with its own next sibling, replacing a node with itself, and sibling-chain/childNodes agreement.

10 of the 12 fail on main. The 2 that pass are the paths that happened to work — the detached early return, and self-replacement, where the reversed arguments coincidentally refer to the same node — kept as guards.

Full suite (195 tests), type-check, and lint are green.

🤖 Generated with Claude Code

replaceWith passed its arguments to replaceChild in the wrong order.
replaceChild(newChild, oldChild) was called as parent.replaceChild(this, node),
naming the incoming node as the child to replace. That node is usually fresh and
has no parent, so the reference check rejected it and every call on a node with a
parent threw "reference node is not a child of this parent". It also read the
following sibling off the incoming node rather than off this, so the remaining
arguments had no correct insertion point to anchor to, and a call with no
arguments stringified undefined into a text node instead of removing the node.

Remove this and insert the given nodes at its position in argument order,
anchored on the first following sibling that is not itself being moved so that
replacing a node with one of its own siblings still has a reference node left.

Co-Authored-By: Claude <noreply@anthropic.com>
@airhorns
airhorns force-pushed the polyfill-replace-with branch from 7a36613 to 1868da7 Compare September 1, 2026 17:50

@andrewiggins andrewiggins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix.

@olavoasantos #681 builds on this with some additional fixes that we need too. So don't close that one - be sure to rebase on top of this and include some of the fixes in there that this PR doesn't have (e.g. ancestor replacement leaves a cycle, throwing argument conversion can leave a partial mutation, custom element callbacks see intermediate state). Let's review those fixes over their PRs.

@andrewiggins
andrewiggins merged commit c3918c6 into Shopify:main Sep 3, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants