-
Notifications
You must be signed in to change notification settings - Fork 46.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Transfer head/body over if appending an HTML document #22876
Conversation
Comparing: c7917fe...eb9a4c6 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
@@ -465,7 +465,49 @@ export function appendChild( | |||
parentInstance: Instance, | |||
child: Instance | TextInstance, | |||
): void { | |||
parentInstance.appendChild(child); | |||
const isDocumentParentNode = parentInstance.nodeType === DOCUMENT_NODE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is changing an extremely hot path and might break a bunch of optimisations. We need to be very careful about this.
@@ -465,7 +465,49 @@ export function appendChild( | |||
parentInstance: Instance, | |||
child: Instance | TextInstance, | |||
): void { | |||
parentInstance.appendChild(child); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the hottest functions in React since it’s for every insertion for every node. It’s also typically inlined. It’s not the right trade off to put it here so we’ll need a different strategy.
Summary
This fixes #22833 by making appending an HTML document instead copy over the attributes of HTML, Head, and Body (preserving the original HTML/Head/Body elements) and then setting the innerHTML of Body/Head.
This stops us from running into
Uncaught DOMException: Failed to execute 'appendChild' on 'Node': Only one element on document allowed
while trying to client render a full page into document.How did you test this change?
Added a jest test
Confirmed issue #22833 is fixed: https://codesandbox.io/s/react-18-hydration-mismatch-in-document-forked-spxnt?file=/src/App.js