Fix ChildNode.replaceWith throwing instead of replacing the node - #623
Merged
Merged
Conversation
JoviDeCroock
approved these changes
Jul 29, 2026
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
force-pushed
the
polyfill-replace-with
branch
from
September 1, 2026 17:50
7a36613 to
1868da7
Compare
andrewiggins
approved these changes
Sep 3, 2026
andrewiggins
left a comment
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
ChildNode.replaceWith()passes its arguments toreplaceChild()in the wrong order. The signature isreplaceChild(newChild, oldChild), but it is called as:replaceChildrejects anoldChildthat 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:Two further consequences of the same mix-up:
nextis read off the incoming (detached) node, so it isnull, and the 2nd..nth arguments have no correct insertion point to anchor to.replaceWith()with no arguments takesnodes[0] === undefinedthroughtoNode, which stringifies it — so instead of removing the node it tries to insert the literal textundefined.Current behaviour on
main, every case with a parent:mainel.replaceWith(fresh)(middle child)el.replaceWith(fresh)(last child)el.replaceWith(a, b)el.replaceWith('text')el.replaceWith()el.replaceWith(fresh)Only the no-parent early return worked, so the method is entirely non-functional.
The fix
Remove
thisand insert the given nodes at its position in argument order: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
whileloop is the spec's viable next sibling. Anchoring onthis[NEXT]alone breaks when a node is replaced with one of its own siblings —b.replaceWith(c)wherecisb's next sibling would movecout of the parent and then try to insert before it, throwing again. Skipping anchors that are themselves being moved keepsb.replaceWith(c)andb.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 inchildNodesbut never enters the forward sibling chain:So
replaceWithcannot 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/childNodesagreement.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, andlintare green.🤖 Generated with Claude Code