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

Move Hydration Warnings from the DOM Config into the Fiber reconciliation #28476

Merged
merged 9 commits into from
Mar 26, 2024

Conversation

sebmarkbage
Copy link
Collaborator

Stacked on #28458.

This doesn't actually really change the messages yet, it's just a refactor.

Hydration warnings can be presented either as HTML or React JSX format. If presented as HTML it makes more sense to make that a DOM specific concept, however, I think it's actually better to present it in terms of React JSX.

Most of the time the errors aren't going to be something messing with them at the HTML/HTTP layer. It's because the JS code does something different. Most of the time you're working in just React. People don't necessarily even know what the HTML form of it looks like. So this takes the approach that the warnings are presented in React JSX in their rich object form.

Therefore, I'm moving the approach to yield diff data to the reconciler but it's the reconciler that's actually printing all the warnings.

@sebmarkbage sebmarkbage requested review from gaearon, gnoff and acdlite March 1, 2024 01:50
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 1, 2024
@react-sizebot
Copy link

react-sizebot commented Mar 1, 2024

Comparing: 84c84d7...21f7e9d

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 +0.01% 175.91 kB 175.92 kB +0.31% 54.53 kB 54.70 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.01% 172.39 kB 172.41 kB +0.26% 53.70 kB 53.84 kB
facebook-www/ReactDOM-prod.classic.js = 589.86 kB 589.78 kB +0.18% 103.60 kB 103.78 kB
facebook-www/ReactDOM-prod.modern.js = 573.39 kB 573.30 kB +0.16% 100.66 kB 100.82 kB
test_utils/ReactAllWarnings.js Deleted 64.83 kB 0.00 kB Deleted 16.08 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactDOM-dev.modern.js +0.29% 1,623.20 kB 1,627.96 kB +0.25% 322.68 kB 323.48 kB
facebook-www/ReactDOMTesting-dev.modern.js +0.29% 1,644.66 kB 1,649.43 kB +0.25% 327.20 kB 328.00 kB
facebook-www/ReactDOM-dev.classic.js +0.29% 1,653.05 kB 1,657.82 kB +0.23% 328.09 kB 328.84 kB
facebook-www/ReactDOMTesting-dev.classic.js +0.28% 1,674.57 kB 1,679.34 kB +0.23% 332.63 kB 333.39 kB
oss-experimental/react-dom/cjs/react-dom.development.js +0.27% 1,305.11 kB 1,308.66 kB +0.24% 288.02 kB 288.71 kB
oss-experimental/react-dom/umd/react-dom.development.js +0.27% 1,368.23 kB 1,371.90 kB +0.24% 290.99 kB 291.68 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.development.js +0.27% 1,322.91 kB 1,326.45 kB +0.22% 292.30 kB 292.94 kB
oss-stable-semver/react-dom/cjs/react-dom.development.js +0.27% 1,314.01 kB 1,317.50 kB +0.24% 289.53 kB 290.22 kB
oss-stable/react-dom/cjs/react-dom.development.js +0.27% 1,314.04 kB 1,317.53 kB +0.24% 289.57 kB 290.26 kB
oss-stable-semver/react-dom/umd/react-dom.development.js +0.26% 1,377.38 kB 1,381.00 kB +0.23% 292.54 kB 293.22 kB
oss-stable/react-dom/umd/react-dom.development.js +0.26% 1,377.41 kB 1,381.02 kB +0.23% 292.57 kB 293.25 kB
oss-experimental/react-reconciler/cjs/react-reconciler.development.js +0.20% 919.84 kB 921.72 kB +0.26% 196.52 kB 197.02 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.development.js +0.20% 923.63 kB 925.51 kB +0.26% 197.31 kB 197.83 kB
oss-stable/react-reconciler/cjs/react-reconciler.development.js +0.20% 923.65 kB 925.53 kB +0.26% 197.34 kB 197.85 kB
oss-stable/react-reconciler/cjs/react-reconciler.profiling.min.js = 123.91 kB 123.33 kB = 37.86 kB 37.78 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.profiling.min.js = 123.88 kB 123.30 kB = 37.84 kB 37.75 kB
oss-experimental/react-reconciler/cjs/react-reconciler.profiling.min.js = 122.13 kB 121.55 kB = 37.41 kB 37.34 kB
oss-stable/react-reconciler/cjs/react-reconciler.production.min.js = 114.86 kB 114.28 kB = 35.53 kB 35.43 kB
oss-stable-semver/react-reconciler/cjs/react-reconciler.production.min.js = 114.84 kB 114.25 kB = 35.50 kB 35.40 kB
oss-experimental/react-reconciler/cjs/react-reconciler.production.min.js = 113.09 kB 112.51 kB = 35.07 kB 35.00 kB
test_utils/ReactAllWarnings.js Deleted 64.83 kB 0.00 kB Deleted 16.08 kB 0.00 kB

Generated by 🚫 dangerJS against 21f7e9d

@sebmarkbage sebmarkbage force-pushed the hydrationwarningsinfiber branch 4 times, most recently from 75422d8 to 6acaed9 Compare March 1, 2024 20:25
Copy link
Collaborator

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

Looks like a test is still not updated for the new warning strings

To do this we need to describe any nodes that don't have a match to diff
against.
This unifies hydrateTextInstance and hydrateInstance so that they both
throw on text mismatches in the same way in prod.

They also get the diff before throwing for DEV.
@sebmarkbage sebmarkbage force-pushed the hydrationwarningsinfiber branch from 2a38b43 to 4a22791 Compare March 26, 2024 21:55
@sebmarkbage sebmarkbage merged commit 4b8dfd6 into facebook:main Mar 26, 2024
38 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 26, 2024
…tion (#28476)

Stacked on #28458.

This doesn't actually really change the messages yet, it's just a
refactor.

Hydration warnings can be presented either as HTML or React JSX format.
If presented as HTML it makes more sense to make that a DOM specific
concept, however, I think it's actually better to present it in terms of
React JSX.

Most of the time the errors aren't going to be something messing with
them at the HTML/HTTP layer. It's because the JS code does something
different. Most of the time you're working in just React. People don't
necessarily even know what the HTML form of it looks like. So this takes
the approach that the warnings are presented in React JSX in their rich
object form.

Therefore, I'm moving the approach to yield diff data to the reconciler
but it's the reconciler that's actually printing all the warnings.

DiffTrain build for [4b8dfd6](4b8dfd6)
kassens added a commit to kassens/react that referenced this pull request Mar 26, 2024
Back out "Move Hydration Warnings from the DOM Config into the Fiber reconciliation (facebook#28476)"

Original commit changeset: 4b8dfd6

Back out "Remove enableClientRenderFallbackOnTextMismatch flag (facebook#28458)"

Original commit changeset: 84c84d7
sebmarkbage added a commit that referenced this pull request Mar 27, 2024
Stacked on #28476.

We used to `console.error` for every mismatch we found, up until the
error we threw for the hydration mismatch.

This changes it so that we build up a set of diffs up until we either
throw or complete hydrating the root/suspense boundary. If we throw, we
append the diff to the error message which gets passed to
onRecoverableError (which by default is also logged to console). If we
complete, we append it to a `console.error`.

Since we early abort when something throws, it effectively means that we
can only collect multiple diffs if there were preceding non-throwing
mismatches - i.e. only properties mismatched but tag name matched.

There can still be multiple logs if multiple siblings Suspense
boundaries all error hydrating but then they're separate errors
entirely.

We still log an extra line about something erroring but I think the goal
should be that it leads to a single recoverable or console.error.

This doesn't yet actually print the diff as part of this message. That's
in a follow up PR.
github-actions bot pushed a commit that referenced this pull request Mar 27, 2024
Stacked on #28476.

We used to `console.error` for every mismatch we found, up until the
error we threw for the hydration mismatch.

This changes it so that we build up a set of diffs up until we either
throw or complete hydrating the root/suspense boundary. If we throw, we
append the diff to the error message which gets passed to
onRecoverableError (which by default is also logged to console). If we
complete, we append it to a `console.error`.

Since we early abort when something throws, it effectively means that we
can only collect multiple diffs if there were preceding non-throwing
mismatches - i.e. only properties mismatched but tag name matched.

There can still be multiple logs if multiple siblings Suspense
boundaries all error hydrating but then they're separate errors
entirely.

We still log an extra line about something erroring but I think the goal
should be that it leads to a single recoverable or console.error.

This doesn't yet actually print the diff as part of this message. That's
in a follow up PR.

DiffTrain build for [f7aa5e0](f7aa5e0)
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…tion (facebook#28476)

Stacked on facebook#28458.

This doesn't actually really change the messages yet, it's just a
refactor.

Hydration warnings can be presented either as HTML or React JSX format.
If presented as HTML it makes more sense to make that a DOM specific
concept, however, I think it's actually better to present it in terms of
React JSX.

Most of the time the errors aren't going to be something messing with
them at the HTML/HTTP layer. It's because the JS code does something
different. Most of the time you're working in just React. People don't
necessarily even know what the HTML form of it looks like. So this takes
the approach that the warnings are presented in React JSX in their rich
object form.

Therefore, I'm moving the approach to yield diff data to the reconciler
but it's the reconciler that's actually printing all the warnings.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…ok#28502)

Stacked on facebook#28476.

We used to `console.error` for every mismatch we found, up until the
error we threw for the hydration mismatch.

This changes it so that we build up a set of diffs up until we either
throw or complete hydrating the root/suspense boundary. If we throw, we
append the diff to the error message which gets passed to
onRecoverableError (which by default is also logged to console). If we
complete, we append it to a `console.error`.

Since we early abort when something throws, it effectively means that we
can only collect multiple diffs if there were preceding non-throwing
mismatches - i.e. only properties mismatched but tag name matched.

There can still be multiple logs if multiple siblings Suspense
boundaries all error hydrating but then they're separate errors
entirely.

We still log an extra line about something erroring but I think the goal
should be that it leads to a single recoverable or console.error.

This doesn't yet actually print the diff as part of this message. That's
in a follow up PR.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…tion (#28476)

Stacked on #28458.

This doesn't actually really change the messages yet, it's just a
refactor.

Hydration warnings can be presented either as HTML or React JSX format.
If presented as HTML it makes more sense to make that a DOM specific
concept, however, I think it's actually better to present it in terms of
React JSX.

Most of the time the errors aren't going to be something messing with
them at the HTML/HTTP layer. It's because the JS code does something
different. Most of the time you're working in just React. People don't
necessarily even know what the HTML form of it looks like. So this takes
the approach that the warnings are presented in React JSX in their rich
object form.

Therefore, I'm moving the approach to yield diff data to the reconciler
but it's the reconciler that's actually printing all the warnings.

DiffTrain build for commit 4b8dfd6.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
Stacked on #28476.

We used to `console.error` for every mismatch we found, up until the
error we threw for the hydration mismatch.

This changes it so that we build up a set of diffs up until we either
throw or complete hydrating the root/suspense boundary. If we throw, we
append the diff to the error message which gets passed to
onRecoverableError (which by default is also logged to console). If we
complete, we append it to a `console.error`.

Since we early abort when something throws, it effectively means that we
can only collect multiple diffs if there were preceding non-throwing
mismatches - i.e. only properties mismatched but tag name matched.

There can still be multiple logs if multiple siblings Suspense
boundaries all error hydrating but then they're separate errors
entirely.

We still log an extra line about something erroring but I think the goal
should be that it leads to a single recoverable or console.error.

This doesn't yet actually print the diff as part of this message. That's
in a follow up PR.

DiffTrain build for commit f7aa5e0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team React 19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants