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

Validate DOM nesting for hydration before the hydration warns / errors #28434

Merged
merged 5 commits into from
Feb 24, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Feb 24, 2024

If there's invalid dom nesting, there will be mismatches following but the nesting is the most important cause of the problem.

Previously we would include the DOM nesting when rerendering thanks to the new model of throw and recovery. However, the log would come during the recovery phase which is after we've already logged that there was a hydration mismatch.

People would consistently miss this log. Which is fair because you should always look at the first log first as the most probable cause.

This ensures that we log in the hydration phase if there's a dom nesting issue. This assumes that the consequence of nesting will appear such that the won't have a mismatch before this. That's typically the case because the node will move up and to be a later sibling. So as long as that happens and we keep hydrating depth first, it should hold true. There might be an issue if there's a suspense boundary between the nodes we'll find discover the new child in the outer path since suspense boundaries as breadth first.

Before:

Screenshot 2024-02-23 at 7 34 01 PM

After:

Screenshot 2024-02-23 at 7 22 24 PM

Cameo: RSC stacks.

If there's invalid dom nesting, there will be mismatches following but
the nesting is the most important cause.

We could also silence the mismatch warning (but not thrown error) too.
This really relies on us throwing to abort hydrating siblings so we don't
get extra mismatches after.
That this is according to the rules of HTML, not React, and that this
will cause a hydration error if not addressed.

"appear as" is a bit academic.
Because otherwise we'll act as if we're already inside the child so
the child is already inside itself.

I don't think we use the context for anything at runtime.
@react-sizebot
Copy link

react-sizebot commented Feb 24, 2024

Comparing: d579e77...353871a

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 = 176.86 kB 176.86 kB = 55.13 kB 55.13 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 179.01 kB 179.01 kB = 55.78 kB 55.77 kB
facebook-www/ReactDOM-prod.classic.js = 592.40 kB 592.40 kB = 104.66 kB 104.66 kB
facebook-www/ReactDOM-prod.modern.js = 575.68 kB 575.68 kB = 101.65 kB 101.66 kB
test_utils/ReactAllWarnings.js Deleted 66.20 kB 0.00 kB Deleted 16.24 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 66.20 kB 0.00 kB Deleted 16.24 kB 0.00 kB

Generated by 🚫 dangerJS against 353871a

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.

I know it’s pedantic but these invalid nestings can only cause hydration issues if you SSR+hydrate. React can actually render these invalid nestings just fine for new trees because there is no parsing involved.

the original message is “react doesn’t support invalid HTML nesting” and the new message is focused more on hydration itself which might make it seem more like this is a hydration problem than just incorrect HTML.

Maybe we the hydration specific warning message during hudration and a non hydration specific message when not hydrating?

@sebmarkbage
Copy link
Collaborator Author

sebmarkbage commented Feb 24, 2024

The issue is that you can get the message while client navigating or loading something, in development, which may then cause a hydration error in production or in another scenario when you MPA nav. That's the reason the warning exists in other cases in the first place. So it's easier to just keep it short.

@sebmarkbage sebmarkbage force-pushed the hydrationnesting branch 3 times, most recently from 0f62354 to 3747687 Compare February 24, 2024 00:50
@gnoff
Copy link
Collaborator

gnoff commented Feb 24, 2024

Ah yeah that makes sense

@sophiebits
Copy link
Collaborator

Nice improvement.

@sebmarkbage sebmarkbage merged commit 118ad2a into facebook:main Feb 24, 2024
37 checks passed
Copy link

@AhmedMuhammedElsaid AhmedMuhammedElsaid left a comment

Choose a reason for hiding this comment

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

Really Nice One ,

EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
facebook#28434)

If there's invalid dom nesting, there will be mismatches following but
the nesting is the most important cause of the problem.

Previously we would include the DOM nesting when rerendering thanks to
the new model of throw and recovery. However, the log would come during
the recovery phase which is after we've already logged that there was a
hydration mismatch.

People would consistently miss this log. Which is fair because you
should always look at the first log first as the most probable cause.

This ensures that we log in the hydration phase if there's a dom nesting
issue. This assumes that the consequence of nesting will appear such
that the won't have a mismatch before this. That's typically the case
because the node will move up and to be a later sibling. So as long as
that happens and we keep hydrating depth first, it should hold true.
There might be an issue if there's a suspense boundary between the nodes
we'll find discover the new child in the outer path since suspense
boundaries as breadth first.

Before:

<img width="996" alt="Screenshot 2024-02-23 at 7 34 01 PM"
src="https://github.com/facebook/react/assets/63648/af70cf7f-898b-477f-be39-13b01cfe585f">

After:

<img width="853" alt="Screenshot 2024-02-23 at 7 22 24 PM"
src="https://github.com/facebook/react/assets/63648/896c6348-1620-4f99-881d-b6069263925e">

Cameo: RSC stacks.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants