Suppress hydration warnings when a preceding sibling suspends #24404

gnoff · 2022-04-19T23:27:41Z

Before this PR if there was a hydration error thrown in a ConcurrentMode tree the hydration procerss would enter a didSuspend state where further errors would be suppressed.

This did not previously happen however when a promise was thrown which would cause a Suspense boundary to suspend. Since it is typical for there to be hydration errors if a preceding sibling suspends because the server rendered content represented by that suspending component is not being claimed the didSuspend state is likely to follow quickly after (on the first non-promise thrown value)

This PR moves where the didSuspend state is activated to be for both Error and Promise thrown values.

closes #24384

sizebot · 2022-04-19T23:31:54Z

Comparing: 0dc4e66...be1cefa

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.52 kB	131.52 kB	=	42.10 kB	42.10 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	136.79 kB	136.79 kB	=	43.68 kB	43.67 kB
facebook-www/ReactDOM-prod.classic.js	=	441.02 kB	441.02 kB	=	80.44 kB	80.43 kB
facebook-www/ReactDOM-prod.modern.js	=	426.28 kB	426.28 kB	=	78.25 kB	78.26 kB
facebook-www/ReactDOMForked-prod.classic.js	=	441.02 kB	441.02 kB	=	80.44 kB	80.44 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against be1cefa

gnoff · 2022-04-20T00:26:17Z

packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js

+        const secondToLastCall =
+          mockError.mock.calls[mockError.mock.calls.length - 2];
+        expect(secondToLastCall).toEqual([
          'Warning: Expected server HTML to contain a matching <%s> in <%s>.%s',
-          'div',
-          'div',
+          'article',
+          'section',
          '\n' +
-            '    in div (at **)\n' +
+            '    in article (at **)\n' +


This warning used to be for trying to match the first Component div against the Text node. It was an expression of sorts of the issue brought up in #24384

This warning now is the intentional mismatch between article and div that the test introduces between client/server renders.

acdlite · 2022-04-20T02:46:25Z

packages/react-reconciler/src/ReactFiberThrow.new.js

@@ -514,8 +518,6 @@ function throwException(
  } else {
    // This is a regular error, not a Suspense wakeable.
    if (getIsHydrating() && sourceFiber.mode & ConcurrentMode) {
-      markDidSuspendWhileHydratingDEV();


I think this was just a mistake and it was meant to go into a Suspense-only path, but it went into an error-only path instead.

What happens if you move this to a Suspense-only path? Like line 455.

Ah i assumed it was intentionally opting into the warning suppression after the first hydration error by using suspend that way and didn't consider it was maybe just an oversight. seems like all tests pass.

I think this is sort of a noop in that by fake suspending on real error you opt out of future warns but the warns already opted out after the first one so in the end no observable difference

I think it might be intentional because if an error is thrown, and nothing suspends, we still continue rendering siblings I believe. So those siblings would still be hydrating and causing more fake errors which would be confusing.

Do we have a test for that?

Really markDidSuspendWhileHydratingDEV is a misnomer because it's more like markDidThrowWhileHydratingDEV.

I don't recall writing a test for the error case but maybe. It does seem like it should be called in both cases, reason I asked whether it passes if you move it to be Suspense-only is I was curious if we already tested that

functionally they are equivelent (I checked in a test) but I agree the didThrow seems to map better onto what we expect to happen after a throw

If you error we would expect hydration to fail for later siblings since we likely didn't consume the current hydration step and will not advance.

If you Suspend we have the exact same logic

The reason this distinction is unobservable is the hydration warning will only ever log once and the errors are not suppressed so you end up with many thrown errors in either case and 1 logged error (didSuspend suppression or didAlreadyLog supression)

I'm in favor of reframing as didThrow and leaving it in both branches so I'll work on that unless someone wants to real me in

Well there is a test to suppress warnings after a hydration error

https://github.com/gnoff/react/blob/hydration-warning-dev/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js#L2993-L3005

Are you concerned about an error thrown from user code?

Yeah I think we should have a test for a user error, too, since the fact that hydration mismatches cause an error internally is an implementation detail; it doesn't have to work that way.

For example, the other way it could work is that we wait to walk the DOM tree until right before the commit phase. Then you'd be able to start rendering even before the HTML has arrived. In that case, we wouldn't know there was a mismatch until after the render phase had already completed. So instead of erroring, we'd throw out the work-in-progress and start again.

Ah yeah that makes sense. I’ll get another test in there soon

Andrew is the new test covering what you hoped it would?

packages/react-reconciler/src/ReactFiberThrow.new.js

packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js

packages/react-reconciler/src/ReactFiberHydrationContext.old.js

packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js

If a components suspends during hydration we expect there to be mismatches with server rendered HTML but we were not always supressing warning messages related to these expected mismatches

previously hydration would only be marked as supsending when a genuine error was thrown. This created an opportunity for a hydration mismatch that would warn after which later hydration mismatches would not lead to warnings. By moving the marker check earlier in the thrownException function we get the hydration context to enter the didSuspend state on both error and thrown promise cases which eliminates this gap.

This test was actually subject to the project identified in the issue fixed in this branch. After fixing the underlying issue the assertion logic needed to change to pick the right warning which now emits after hydration successfully completes on promise resolution. I changed the container type to 'section' to make the error message slightly easier to read/understand (for me)

For unknown reasons the didSuspend was being set only on the error path and nto the suspense path. The original change hoisted this to happen on both paths. This change moves the didSuspend call to the suspense path only. This appears to be a noop because if the error path marked didSuspend it would suppress later warnings but if it does not the warning functions themsevles do that suppression (after the first one which necessarily already happened)

the orignial behavior applied the hydration warning bailout to error paths only. originally I moved it to Suspense paths only but this commit restores it to both paths and renames the marker function as didThrow rather than didSuspend The logic here is that for either case if we get a mismatch in hydration we want to warm up components but effectively consider the hydration for this boundary halted

…hould only affect dev behavior

gaearon · 2022-04-20T19:46:54Z

@gnoff Is this ready to merge or do you plan to add the test for the error case?

gnoff · 2022-04-20T20:04:15Z

I was going to add a test still. Should have something in the next 30 min

it not has an intentional client mismatch that would error if there wasn't supression brought about by the earlier error. when it client rendres it has the updated value not found in the server response but we do not see a hydration warning because it was superseded by the thrown error in that render

xiel · 2022-04-21T07:08:07Z

@gnoff, @gaearon and Team, thanks for working on the issue so swiftly!

Is there a package version built that I can test before release?
These ones are using (.../commit/be1cefa0/react), but I am still seening the hydration bug issue #24384 was about.

Is this pulling and using the correct package?

In my view there should be no warning/error at all, because both client&server resolve to the exact same tree.
Or what am I misunderstanding?

I also noticed that while the component is suspended, and you move your mouse, the console is flooded with console errors:
Uncaught Error: Hydration failed because the initial UI does not match what was rendered on the server.
https://user-images.githubusercontent.com/615522/164394433-9635c494-ba12-470c-833b-68b81c7c481a.mov

xiel · 2022-04-21T07:10:56Z

packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js

@@ -2841,4 +2841,247 @@ describe('ReactDOMFizzServer', () => {
      expect(window.__test_outlet).toBe(1);
    });
  });
+
+  // @gate experimental && enableClientRenderFallbackOnTextMismatch
+  it('#24384: Suspending should halt hydration warnings while still allowing siblings to warm up', async () => {


Can we add a test, where there is no mismatch at all between client and server (and no emitted warnings)?

yes, thanks: #24436

gnoff · 2022-04-22T00:59:56Z

@xiel you are right that doesn’t make a lot of sense. Going to take a look further

gnoff · 2022-04-22T19:16:47Z

the surprising behavior is that if within a suspense boundary you only error and do not suspend we currently suppress all hydration warnings but all hydration errors get upgraded to recoverable on root completion.

This is happening because when you suspend the client forcedClientRender flag is off and the tree ends up not committing. and when you re-render on ping we no longer expect the suspend to cause hydration problems so we log all warnings and errors as expected

But with an error only within a suspense boundary we continue rendering siblings accruing potentially many hydration errors but because we force a client re-render we never purge the hydration errors queue and when the thing finally commits everything gets upgraded to recoverable.

So my current thinking is

desired behavior

If there are userland hyration errors keep them along with There was an error while hydrating this Suspense boundary. Switched to client rendering. error but drop the hydration diff errors
If there are no userland hydration errors Keep everything

possible approach

If there are userland errors as hydrationErrors drop them from hydrationErrors queue before calling retrySuspenseComponentWithoutHydrating

(easier said than done)

In facebook#24404 a test case was added for supressing hydration warnings if a preceding component suspends. The test case showed that hydration warnings would still emit after resolution but it did not include a test case for when the resolution led to a complete hydration without errors. This commit adds the additional test case demonstrating that no warnings or Errors are observed if hydration completes successfully after suspended component resolution

gnoff · 2022-04-25T16:22:43Z

desired behavior
* If there are userland hyration errors keep them along with `There was an error while hydrating this Suspense boundary. Switched to client rendering.` error but drop the hydration diff errors

* If there are no userland hydration errors Keep everything
possible approach

If there are userland errors as hydrationErrors drop them from hydrationErrors queue before calling retrySuspenseComponentWithoutHydrating

I took a slightly different implementation approach in #24427 but behavior should match what I wrote above

Summary: This sync includes the following changes: - **[bd4784c8f](facebook/react@bd4784c8f )**: Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) ([#24434](facebook/react#24434)) //<dan>// - **[6d3b6d0f4](facebook/react@6d3b6d0f4 )**: forwardRef et al shouldn't affect if props reused ([#24421](facebook/react#24421)) //<Andrew Clark>// - **[bd0813766](facebook/react@bd0813766 )**: Fix: useDeferredValue should reuse previous value ([#24413](facebook/react#24413)) //<Andrew Clark>// - **[9ae80d6a2](facebook/react@9ae80d6a2 )**: Suppress hydration warnings when a preceding sibling suspends ([#24404](facebook/react#24404)) //<Josh Story>// - **[0dc4e6663](facebook/react@0dc4e6663 )**: Land enableClientRenderFallbackOnHydrationMismatch ([#24410](facebook/react#24410)) //<Andrew Clark>// - **[354772952](facebook/react@354772952 )**: Land enableSelectiveHydration flag ([#24406](facebook/react#24406)) //<Andrew Clark>// - **[392808a1f](facebook/react@392808a1f )**: Land enableClientRenderFallbackOnTextMismatch flag ([#24405](facebook/react#24405)) //<Andrew Clark>// - **[1e748b452](facebook/react@1e748b452 )**: Land enableLazyElements flag ([#24407](facebook/react#24407)) //<Andrew Clark>// - **[4175f0593](facebook/react@4175f0593 )**: Temporarily feature flag numeric fallback for symbols ([#24401](facebook/react#24401)) //<Ricky>// - **[a6d53f346](facebook/react@a6d53f346 )**: Revert "Clean up Selective Hydration / Event Replay flag ([#24156](facebook/react#24156))" ([#24402](facebook/react#24402)) //<Ricky>// - **[ab9cdd34f](facebook/react@ab9cdd34f )**: Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted ([#24400](facebook/react#24400)) //<Andrew Clark>// - **[168da8d55](facebook/react@168da8d55 )**: Fix typo that happened during rebasing //<Andrew Clark>// - **[8bc527a4c](facebook/react@8bc527a4c )**: Bugfix: Fix race condition between interleaved and non-interleaved updates ([#24353](facebook/react#24353)) //<Andrew Clark>// - **[f7cf077cc](facebook/react@f7cf077cc )**: [Transition Tracing] Add Offscreen Queue ([#24341](facebook/react#24341)) //<Luna Ruan>// - **[4fc394bbe](facebook/react@4fc394bbe )**: Fix suspense fallback throttling ([#24253](facebook/react#24253)) //<sunderls>// - **[80170a068](facebook/react@80170a068 )**: Match bundle.name and match upper case entry points ([#24346](facebook/react#24346)) //<Sebastian Markbåge>// - **[fea6f8da6](facebook/react@fea6f8da6 )**: [Transition Tracing] Add transition to OffscreenState and pendingSuspenseBoundaries to RootState ([#24340](facebook/react#24340)) //<Luna Ruan>// - **[8e2f9b086](facebook/react@8e2f9b086 )**: move passive flag ([#24339](facebook/react#24339)) //<Luna Ruan>// - **[55a21ef7e](facebook/react@55a21ef7e )**: fix pushTransition for transition tracing ([#24338](facebook/react#24338)) //<Luna Ruan>// - **[069d23bb7](facebook/react@069d23bb7 )**: [eslint-plugin-exhaustive-deps] Fix exhaustive deps check for unstable vars ([#24343](facebook/react#24343)) //<Afzal Sayed>// - **[4997515b9](facebook/react@4997515b9 )**: Point useSubscription to useSyncExternalStore shim ([#24289](facebook/react#24289)) //<dan>// - **[01e2bff1d](facebook/react@01e2bff1d )**: Remove unnecessary check ([#24332](facebook/react#24332)) //<zhoulixiang>// - **[d9a0f9e20](facebook/react@d9a0f9e20 )**: Delete create-subscription folder ([#24288](facebook/react#24288)) //<dan>// - **[f993ffc51](facebook/react@f993ffc51 )**: Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue ([#24247](facebook/react#24247)) //<Andrew Clark>// - **[fa5800226](facebook/react@fa5800226 )**: [Fizz] Pipeable Stream Perf ([#24291](facebook/react#24291)) //<Josh Story>// - **[0568c0f8c](facebook/react@0568c0f8c )**: Replace zero with NoLanes for consistency in FiberLane ([#24327](facebook/react#24327)) //<Leo>// - **[e0160d50c](facebook/react@e0160d50c )**: add transition tracing transitions stack ([#24321](facebook/react#24321)) //<Luna Ruan>// - **[b0f13e5d3](facebook/react@b0f13e5d3 )**: add pendingPassiveTransitions ([#24320](facebook/react#24320)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 60e63b9...bd4784c jest_e2e[run_all_tests] Reviewed By: kacieb Differential Revision: D35899012 fbshipit-source-id: 86a885e336fca9f0efa80cd2b8ca040f2cb53853

facebook-github-bot added the CLA Signed label Apr 19, 2022

gnoff force-pushed the hydration-warning-dev branch 2 times, most recently from ef55aca to cc0158c Compare April 20, 2022 00:22

gnoff commented Apr 20, 2022

View reviewed changes

gnoff requested review from acdlite, gaearon and sebmarkbage April 20, 2022 00:27

acdlite reviewed Apr 20, 2022

View reviewed changes

gaearon reviewed Apr 20, 2022

View reviewed changes

packages/react-reconciler/src/ReactFiberThrow.new.js Outdated Show resolved Hide resolved

gaearon mentioned this pull request Apr 20, 2022

Changelog for 18.1 #24411

Merged

gnoff force-pushed the hydration-warning-dev branch from 1d103d0 to f3cdd36 Compare April 20, 2022 18:13

gaearon reviewed Apr 20, 2022

View reviewed changes

packages/react-dom/src/__tests__/ReactDOMFizzSuppressHydrationWarning-test.js Outdated Show resolved Hide resolved

gaearon reviewed Apr 20, 2022

View reviewed changes

packages/react-reconciler/src/ReactFiberHydrationContext.old.js Outdated Show resolved Hide resolved

gaearon reviewed Apr 20, 2022

View reviewed changes

packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js Outdated Show resolved Hide resolved

gnoff added 10 commits April 20, 2022 11:31

Add failing test case for facebook#24384

a9ccb8b

If a components suspends during hydration we expect there to be mismatches with server rendered HTML but we were not always supressing warning messages related to these expected mismatches

gate test on hydration fallback flags

5e1cd0a

factor tests to assert different behavior between prod and dev

3bccb4d

add DEV suffix to didSuspendOrError to better indicate this feature s…

eeede50

…hould only affect dev behavior

move tests back to ReactDOMFizzServer-test

c827582

fix comment casing

945352e

gnoff force-pushed the hydration-warning-dev branch from 851dde2 to 6ddb123 Compare April 20, 2022 18:33

gaearon approved these changes Apr 20, 2022

View reviewed changes

drop extra flag gates in tests

07a2eee

gnoff force-pushed the hydration-warning-dev branch from 6ddb123 to 07a2eee Compare April 20, 2022 18:42

gnoff added 3 commits April 20, 2022 14:31

add test for user error case

8839f1b

remove unnecessary gate

3fd1eb6

gnoff merged commit 9ae80d6 into facebook:main Apr 20, 2022

gnoff deleted the hydration-warning-dev branch April 20, 2022 21:52

xiel reviewed Apr 21, 2022

View reviewed changes

xiel mentioned this pull request Apr 21, 2022

Bug: Incorrect Hydration Mismatch Detection during Suspense - "Hydration failed because the initial UI does not match what was rendered on the server." #24384

Closed

gaearon mentioned this pull request Apr 25, 2022

Avoid accumulating hydration mismatch errors after the first hydration error #24427

Closed

gnoff mentioned this pull request Apr 25, 2022

Add test case for hydration warning suppression #24436

Closed

spudly mentioned this pull request Mar 2, 2023

Bug: lazy + hydrateRoot + sibling = mismatch #26181

Closed

cssecautomation mentioned this pull request Mar 3, 2023

[Snyk] Upgrade: react, react-dom OutSystems/ReactView#126

Open

cssecautomation mentioned this pull request Mar 11, 2023

[Snyk] Upgrade: react, react-dom OutSystems/ReactView#136

Open

This was referenced Feb 10, 2024

[Snyk] Upgrade: react, react-dom stack-spot/web-react-appshell-template#4

Open

[Snyk] Upgrade: react, react-dom stack-spot/web-nextjs-template#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suppress hydration warnings when a preceding sibling suspends #24404

Suppress hydration warnings when a preceding sibling suspends #24404

gnoff commented Apr 19, 2022 •

edited

sizebot commented Apr 19, 2022 •

edited

gnoff Apr 20, 2022

acdlite Apr 20, 2022

gnoff Apr 20, 2022

sebmarkbage Apr 20, 2022 •

edited

acdlite Apr 20, 2022

gnoff Apr 20, 2022

gnoff Apr 20, 2022 •

edited

acdlite Apr 20, 2022

acdlite Apr 20, 2022

gnoff Apr 20, 2022

gnoff Apr 20, 2022

gaearon commented Apr 20, 2022

gnoff commented Apr 20, 2022

xiel commented Apr 21, 2022 •

edited

xiel Apr 21, 2022 •

edited

gnoff Apr 25, 2022

gnoff commented Apr 22, 2022

gnoff commented Apr 22, 2022

gnoff commented Apr 25, 2022

desired behavior

possible approach

Suppress hydration warnings when a preceding sibling suspends #24404

Suppress hydration warnings when a preceding sibling suspends #24404

Conversation

gnoff commented Apr 19, 2022 • edited

sizebot commented Apr 19, 2022 • edited

Critical size changes

Significant size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnoff Apr 20, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Apr 20, 2022

gnoff commented Apr 20, 2022

xiel commented Apr 21, 2022 • edited

xiel Apr 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnoff commented Apr 22, 2022

gnoff commented Apr 22, 2022

desired behavior

possible approach

gnoff commented Apr 25, 2022

desired behavior

possible approach

gnoff commented Apr 19, 2022 •

edited

sizebot commented Apr 19, 2022 •

edited

sebmarkbage Apr 20, 2022 •

edited

gnoff Apr 20, 2022 •

edited

xiel commented Apr 21, 2022 •

edited

xiel Apr 21, 2022 •

edited