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

Don't mute hydration errors forcing client render #24276

Merged
merged 2 commits into from
Apr 5, 2022

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 5, 2022

tldr: suppressHydrationWarning shouldn't mute warnings that force a client render.

Before 18 + createRoot, all hydration mismatches except prop mismatches were patched up. Prop mismatches were warned about but not patched up. And suppressHydrationWarning on the parent was a universal way to suppress messages — about all kinds of mismatches.

In 18 + createRoot, mismatches always revert to a clean render up to the closest <Suspense> above except for one case. The exception is suppressHydrationWarning: it enables patching for text mismatches.

However, this means that you can't debug other kinds of mismatches inside suppressHydrationWarning. Insertions and deletions now revert to client render but we still mute the warning because in 17 it was harmless to mute. We've introduced limited production behavior for the prop, but we haven't adjusted the warning messages themselves to show for the case where mismatches are not patched up.

This fixes the issue.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 5, 2022
@@ -248,9 +248,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Expected server HTML to contain a matching <span> in <span>',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These were being covered up, making mismatches with prod behavior hard to debug and fix.

@sizebot
Copy link

sizebot commented Apr 5, 2022

Comparing: 5f7f528...380dc5b

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.39 kB 131.40 kB = 41.99 kB 41.98 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.44 kB 136.45 kB = 43.43 kB 43.43 kB
facebook-www/ReactDOM-prod.classic.js +0.03% 432.94 kB 433.07 kB +0.01% 79.59 kB 79.60 kB
facebook-www/ReactDOM-prod.modern.js +0.03% 417.94 kB 418.07 kB +0.01% 77.23 kB 77.24 kB
facebook-www/ReactDOMForked-prod.classic.js +0.03% 432.94 kB 433.07 kB +0.01% 79.60 kB 79.61 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 380dc5b

@@ -248,9 +248,10 @@ describe('ReactDOMFizzServerHydrationWarning', () => {
]);
}).toErrorDev(
[
'Expected server HTML to contain a matching <span> in <span>',
'An error occurred during hydration. The server HTML was replaced with client content in <div>.',
Copy link
Collaborator

@sebmarkbage sebmarkbage Apr 5, 2022

Choose a reason for hiding this comment

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

You still have these though. Isn't it better to have the context here for the ones we check in prod anyway? Is it worth having both? Is it confusing that there are two with subtly different timing?

Maybe it would make more sense to have the DEV only error if it was part of a larger diff across multiple mismatches which also included attributes.

Copy link
Collaborator

@sebmarkbage sebmarkbage Apr 5, 2022

Choose a reason for hiding this comment

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

In other words, should we just move the insert/deletion error messages to the onRecoverableError? Perhaps batching them up if there are multiple. Oops, now you just got nerd sniped into making a larger diff view.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have another PR in the works for the diff. It's not coalesced though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But in either case I think we should fix up the inconsistency first. I.e. your concerns don't seem related to this PR per se, it just aligns the behavior for suppressHydration ones with the rest.

@gaearon gaearon merged commit 9ededef into facebook:main Apr 5, 2022
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 12, 2022
Summary:
This sync includes the following changes:
- **[e8f4a6653](facebook/react@e8f4a6653 )**: Fix import in example //<dan>//
- **[bb49abea2](facebook/react@bb49abea2 )**: Update some READMEs ([#24290](facebook/react#24290)) //<dan>//
- **[4bc465a16](facebook/react@4bc465a16 )**: Rename Controls to PipeableStream ([#24286](facebook/react#24286)) //<Sebastian Markbåge>//
- **[ece5295e5](facebook/react@ece5295e5 )**: Remove unnecessary flag check ([#24284](facebook/react#24284)) //<zhoulixiang>//
- **[9ededef94](facebook/react@9ededef94 )**: Don't mute hydration errors forcing client render ([#24276](facebook/react#24276)) //<dan>//
- **[985272e26](facebook/react@985272e26 )**: Fix name mismatch in react-reconciler custom build. ([#24272](facebook/react#24272)) //<Hikari Hayashi>//
- **[b8cfda15e](facebook/react@b8cfda15e )**: changed Transitions type to Array<Transition> ([#24249](facebook/react#24249)) //<Luna Ruan>//
- **[c89a15c71](facebook/react@c89a15c71 )**: [ReactDebugTools] wrap uncaught error from rendering user's component ([#24216](facebook/react#24216)) //<Mengdi "Monday" Chen>//
- **[ebd7ff65b](facebook/react@ebd7ff65b )**: Don't recreate the same fallback on the client if hydrating suspends ([#24236](facebook/react#24236)) //<dan>//
- **[aa05e7315](facebook/react@aa05e7315 )**: Add 4.4.0 release to eslint rules CHANGELOG ([#24234](facebook/react#24234)) //<Brian Vaughn>//
- **[7e3121e1c](facebook/react@7e3121e1c )**: Remove unstable_createMutableSource from experimental build ([#24209](facebook/react#24209)) //<Sebastian Silbermann>//
- **[0415b18a1](facebook/react@0415b18a1 )**: [ReactDebugTools] add custom error type for future new hooks ([#24168](facebook/react#24168)) //<Mengdi "Monday" Chen>//

Changelog:
[General][Changed] - React Native sync for revisions 34aa5cf...e8f4a66

jest_e2e[run_all_tests]

Reviewed By: kacieb

Differential Revision: D35581059

fbshipit-source-id: ee031dfc49b1ef663b601f33f3f3f6c5a804971e
rickhanlonii pushed a commit that referenced this pull request Apr 13, 2022
* Don't mute hydration errors forcing client render

* Nits
rickhanlonii pushed a commit that referenced this pull request Apr 14, 2022
* Don't mute hydration errors forcing client render

* Nits
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* Don't mute hydration errors forcing client render

* Nits
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This sync includes the following changes:
- **[e8f4a6653](facebook/react@e8f4a6653 )**: Fix import in example //<dan>//
- **[bb49abea2](facebook/react@bb49abea2 )**: Update some READMEs ([facebook#24290](facebook/react#24290)) //<dan>//
- **[4bc465a16](facebook/react@4bc465a16 )**: Rename Controls to PipeableStream ([facebook#24286](facebook/react#24286)) //<Sebastian Markbåge>//
- **[ece5295e5](facebook/react@ece5295e5 )**: Remove unnecessary flag check ([facebook#24284](facebook/react#24284)) //<zhoulixiang>//
- **[9ededef94](facebook/react@9ededef94 )**: Don't mute hydration errors forcing client render ([facebook#24276](facebook/react#24276)) //<dan>//
- **[985272e26](facebook/react@985272e26 )**: Fix name mismatch in react-reconciler custom build. ([facebook#24272](facebook/react#24272)) //<Hikari Hayashi>//
- **[b8cfda15e](facebook/react@b8cfda15e )**: changed Transitions type to Array<Transition> ([facebook#24249](facebook/react#24249)) //<Luna Ruan>//
- **[c89a15c71](facebook/react@c89a15c71 )**: [ReactDebugTools] wrap uncaught error from rendering user's component ([facebook#24216](facebook/react#24216)) //<Mengdi "Monday" Chen>//
- **[ebd7ff65b](facebook/react@ebd7ff65b )**: Don't recreate the same fallback on the client if hydrating suspends ([facebook#24236](facebook/react#24236)) //<dan>//
- **[aa05e7315](facebook/react@aa05e7315 )**: Add 4.4.0 release to eslint rules CHANGELOG ([facebook#24234](facebook/react#24234)) //<Brian Vaughn>//
- **[7e3121e1c](facebook/react@7e3121e1c )**: Remove unstable_createMutableSource from experimental build ([facebook#24209](facebook/react#24209)) //<Sebastian Silbermann>//
- **[0415b18a1](facebook/react@0415b18a1 )**: [ReactDebugTools] add custom error type for future new hooks ([facebook#24168](facebook/react#24168)) //<Mengdi "Monday" Chen>//

Changelog:
[General][Changed] - React Native sync for revisions 34aa5cf...e8f4a66

jest_e2e[run_all_tests]

Reviewed By: kacieb

Differential Revision: D35581059

fbshipit-source-id: ee031dfc49b1ef663b601f33f3f3f6c5a804971e
rickhanlonii added a commit that referenced this pull request Feb 1, 2024
## Overview

Branched off #28130

## Why

In #24276 we changed the new root
behavior to error when a client-render is forced for certain cases, so
these now expect a mismatch even though they're using
`suppressHydrationWarning`.
gaearon pushed a commit that referenced this pull request Feb 3, 2024
## Overview

Branched off #28130

## Why

In #24276 we changed the new root
behavior to error when a client-render is forced for certain cases, so
these now expect a mismatch even though they're using
`suppressHydrationWarning`.
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.

4 participants