Make removeChild in receivers less strict#533
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
igor10k
force-pushed
the
i10k/less_strict_remove_child
branch
from
ae7c371 to
94ab3c1
Compare
thomas-marcucci
approved these changes
Feb 21, 2025
thomas-marcucci
left a comment
thomas-marcucci
left a comment
There was a problem hiding this comment.
This makes sense to me! Ideally I guess there would be tests, but at a glance I don't see any so maybe it's not an issue here.
simontaisne
approved these changes
Feb 24, 2025
Member
simontaisne
left a comment
simontaisne
left a comment
There was a problem hiding this comment.
LGTM, thanks for looking into that 💯
lemonmade
approved these changes
Feb 24, 2025
Merged
shopify-river Bot
pushed a commit
that referenced
this pull request
Apr 30, 2026
Drop `insertChild`, `removeChild`, `updateProperty`, and `updateText` mutations whose target node is no longer in the receiver's `attached` map, instead of throwing a `TypeError` while dereferencing the missing node. This race surfaces in production as unhandled promise rejections such as `TypeError: undefined is not an object (evaluating 'x.properties')` (Safari) / `TypeError: Cannot read properties of undefined (reading 'properties')` (V8) when a remote sender dispatches a mutation for a node whose host-side state has just been removed. PR #533 previously added a similar guard to `removeChild` for the case where the *child slot* at a given index was empty, but did not handle the case where the *parent itself* was missing, and did not touch `updateProperty` or `updateText`. This change applies the same defensive pattern uniformly to every connection callback that dereferences `attached.get(id)`. Late mutations targeting a detached subtree are by definition no-ops: there is nothing left to mutate. Fixes the original report in #542 (which #533 was assumed to fix but did not actually cover). Requested by Eddie Chan <eddie.chan@shopify.com>
eddiechan
added a commit
that referenced
this pull request
May 8, 2026
Drop `insertChild`, `removeChild`, `updateProperty`, and `updateText` mutations whose target node is no longer in the receiver's `attached` map, instead of throwing a `TypeError` while dereferencing the missing node. This race surfaces in production as unhandled promise rejections such as `TypeError: undefined is not an object (evaluating 'x.properties')` (Safari) / `TypeError: Cannot read properties of undefined (reading 'properties')` (V8) when a remote sender dispatches a mutation for a node whose host-side state has just been removed. PR #533 previously added a similar guard to `removeChild` for the case where the *child slot* at a given index was empty, but did not handle the case where the *parent itself* was missing, and did not touch `updateProperty` or `updateText`. This change applies the same defensive pattern uniformly to every connection callback that dereferences `attached.get(id)`. Late mutations targeting a detached subtree are by definition no-ops: there is nothing left to mutate. Fixes the original report in #542 (which #533 was assumed to fix but did not actually cover). Requested by Eddie Chan <eddie.chan@shopify.com>
Merged
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
4 participants
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.
Porting #522 since through the adapter
remote-domreceivers can be used withremote-uiremote which can be using old versions ofremote-uithat don't handleremoveChildcalls well.