Skip to content
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

Support document rendering #24523

Merged
merged 2 commits into from
May 10, 2022
Merged

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented May 9, 2022

Hydrating into Document

This PR addressed the most fundamental issue identified in #22833. In that issue a hydration mismatch when hydrating into the Document caused a broken application because the client rendered fallback could not append any content to the Document.

This is caused by the fact that a Document can only have 1 element at a time, namely the documentElement which is usually <html> and by not removing it beforehand the append of a new <html> element fails.

Initially I looked into recycling the documentElement so it is never removed or appended but just moved around to whatever fiber needs it, staying in the Document tree as its root. This worked fine but added some code on hot paths and a bit of size.

I then looked at what would happen if we simply removed the documentElement (i.e. treat it like any other Element) and in Chrome, Safari, and Firefox (modern versions) there seems to be no issue with this. This PR is much smaller and simply updates clearContainer to properly clear the Document as a container so that a later appendChild of an <html> element will not violation Document invariants

Hydration into Document compatibility with 3rd party scripts and Extensions

The original issue shows an extension breaking a React application. This PR does not introduce compatibility with extensions or 3rd party scripts that modify the DOM before React hydrates, it simply inverts what gets broken. This change would result in (worst case) React breaking the extension rather than the extension breaking React.

createRoot for Document

as a consequence of the fix for hydration in Document using Document as a root should work now. This PR also adds back types for using createRoot on Document. It should be noted that doing so will effectively wipe out the entire document so third party styles and other DOM elements created by server response, 3rd party scripts, or extensions will be dropped. This is probably not that useful a feature but since it is supported by implementation with no special casing I added back in

gnoff added 2 commits May 9, 2022 09:02
Previously Document was not handled effectively as a container. in particual when hydrating if there was a fallback to client rendering React would attempt to append a new <html> element into the document before clearing out the existing one which errored leaving the application in brokent state.

The initial approach I took was to recycle the documentElement and never remove or append it, always just moving it to the right fiber and appending the right children (heady/body) as needed. However in testing a simple approach in modern browsers it seems like treating the documentElement like any other element works fine. This change modifies the clearContainer method to remove the documentElement if the container is a DOCUMENT_NODE. Once the container is cleared React can append a new documentElement via normal means.
previously rendering into Document was broken and only hydration worked because React did not properly deal with the documentElement and would error in a broken state if used that way. With the previous commit addressing this limitation this change re-adds Document as a valid container for createRoot.

It should be noted that if you use document with createRoot it will drop anything a 3rd party scripts adds the page before rendering for the first time.
@sizebot
Copy link

sizebot commented May 9, 2022

Comparing: d20c3af...0215c83

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.58 kB 131.58 kB = 42.15 kB 42.15 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.82 kB 136.82 kB = 43.70 kB 43.68 kB
facebook-www/ReactDOM-prod.classic.js = 440.68 kB 440.68 kB = 80.35 kB 80.33 kB
facebook-www/ReactDOM-prod.modern.js = 425.89 kB 425.89 kB = 78.17 kB 78.15 kB
facebook-www/ReactDOMForked-prod.classic.js = 440.68 kB 440.68 kB = 80.35 kB 80.33 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 0215c83

@sebmarkbage sebmarkbage requested a review from eps1lon May 9, 2022 16:31
@eps1lon
Copy link
Collaborator

eps1lon commented May 10, 2022

This PR addressed the most fundamental issue identified in #22833.

Does this PR actually fix #22833?

@gnoff
Copy link
Collaborator Author

gnoff commented May 10, 2022

This PR addressed the most fundamental issue identified in #22833.

Does this PR actually fix #22833?

Yes, https://codesandbox.io/s/react-18-hydration-mismatch-in-document-fixed-24523-9n6zos?file=/package.json

There are hydration errors but the issue of unmounting the entire app is fixed. The hydration errors are expected if I understand the test case correctly

@CanRau
Copy link

CanRau commented May 12, 2022

Any eta on when this will be shipped?

@gaearon
Copy link
Collaborator

gaearon commented May 12, 2022

Hopefully within the next two weeks.

@ZipBrandon
Copy link

Looking to check on an updated timeline estimate for this and 18.2? This PR resolves issues that I experience with SSR, but other libraries which mark React as a peer dependency are upset when I am having to npm i --force the next for react and react-dom. The sub-packages end up installing their own React dependencies which leads to the Rules of Hooks being broken (like https://github.com/plouc/nivo/blob/master/packages/bar/package.json) which I have to subsequently remove from its node_modules.

@gaearon
Copy link
Collaborator

gaearon commented Jun 3, 2022

You can use Yarn resolutions or npm overrides to force "deep" packages to use the same version, as far as I know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants