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 Mismatch Errors to Throw or Log Once (Kind of) #28502

Merged
merged 10 commits into from
Mar 27, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Mar 6, 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.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 6, 2024
@react-sizebot
Copy link

react-sizebot commented Mar 6, 2024

Comparing: 4b8dfd6...ec5901d

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 = 175.92 kB 175.85 kB = 54.70 kB 54.68 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 172.41 kB 172.33 kB = 53.84 kB 53.81 kB
facebook-www/ReactDOM-prod.classic.js = 589.78 kB 589.61 kB = 103.78 kB 103.77 kB
facebook-www/ReactDOM-prod.modern.js = 573.30 kB 573.13 kB = 100.82 kB 100.81 kB
test_utils/ReactAllWarnings.js Deleted 65.27 kB 0.00 kB Deleted 16.17 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
test_utils/ReactAllWarnings.js Deleted 65.27 kB 0.00 kB Deleted 16.17 kB 0.00 kB

Generated by 🚫 dangerJS against ec5901d

@sebmarkbage
Copy link
Collaborator Author

Stamped downstream.

@sebmarkbage sebmarkbage merged commit f7aa5e0 into facebook:main Mar 27, 2024
37 of 38 checks passed
sebmarkbage added a commit that referenced this pull request Mar 27, 2024
Stacked on #28502.

This builds on the mechanism in #28502 by adding a diff of everything
we've collected so far to the thrown error or logged error.

This isn't actually a longest common subsequence diff. This means that
there are certain cases that can appear confusing such as a node being
added/removed when it really would've appeared later in the list. In
fact once a node mismatches, we abort rendering so we don't have the
context of what would've been rendered. It's not quite right to use the
result of the recovery render because it can use client-only code paths
using useSyncExternalStore which would yield false differences. That's
why diffing the HTML isn't quite right.

I also present abstract components in the stack, these are presented
with the client props and no diff since we don't have the props that
were on the server. The lack of difference might be confusing but it's
useful for context.

The main thing that's data new here is that we're adding some siblings
and props for context.

Examples in the [snapshot
commit](e14532f).
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)
github-actions bot pushed a commit that referenced this pull request Mar 27, 2024
Stacked on #28502.

This builds on the mechanism in #28502 by adding a diff of everything
we've collected so far to the thrown error or logged error.

This isn't actually a longest common subsequence diff. This means that
there are certain cases that can appear confusing such as a node being
added/removed when it really would've appeared later in the list. In
fact once a node mismatches, we abort rendering so we don't have the
context of what would've been rendered. It's not quite right to use the
result of the recovery render because it can use client-only code paths
using useSyncExternalStore which would yield false differences. That's
why diffing the HTML isn't quite right.

I also present abstract components in the stack, these are presented
with the client props and no diff since we don't have the props that
were on the server. The lack of difference might be confusing but it's
useful for context.

The main thing that's data new here is that we're adding some siblings
and props for context.

Examples in the [snapshot
commit](e14532f).

DiffTrain build for [2ec2aae](2ec2aae)
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.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
Stacked on facebook#28502.

This builds on the mechanism in facebook#28502 by adding a diff of everything
we've collected so far to the thrown error or logged error.

This isn't actually a longest common subsequence diff. This means that
there are certain cases that can appear confusing such as a node being
added/removed when it really would've appeared later in the list. In
fact once a node mismatches, we abort rendering so we don't have the
context of what would've been rendered. It's not quite right to use the
result of the recovery render because it can use client-only code paths
using useSyncExternalStore which would yield false differences. That's
why diffing the HTML isn't quite right.

I also present abstract components in the stack, these are presented
with the client props and no diff since we don't have the props that
were on the server. The lack of difference might be confusing but it's
useful for context.

The main thing that's data new here is that we're adding some siblings
and props for context.

Examples in the [snapshot
commit](facebook@e14532f).
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.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
Stacked on #28502.

This builds on the mechanism in #28502 by adding a diff of everything
we've collected so far to the thrown error or logged error.

This isn't actually a longest common subsequence diff. This means that
there are certain cases that can appear confusing such as a node being
added/removed when it really would've appeared later in the list. In
fact once a node mismatches, we abort rendering so we don't have the
context of what would've been rendered. It's not quite right to use the
result of the recovery render because it can use client-only code paths
using useSyncExternalStore which would yield false differences. That's
why diffing the HTML isn't quite right.

I also present abstract components in the stack, these are presented
with the client props and no diff since we don't have the props that
were on the server. The lack of difference might be confusing but it's
useful for context.

The main thing that's data new here is that we're adding some siblings
and props for context.

Examples in the [snapshot
commit](e14532f).

DiffTrain build for commit 2ec2aae.
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