Don't recreate the same fallback on the client if hydrating suspends #24236

gaearon · 2022-03-31T21:02:03Z

If the server errors (or if it suspends in renderToString, which is like erroring — there's never gonna be more content) within a Suspense boundary, we send its fallback to the client with a special marker. The client is supposed to retry rendering that Suspense boundary's content during hydration with a clean render and replace the DOM.

The question is what happens if hydrating suspends. Previously, we'd use the result of that clean render. However, the result is the same fallback. So we'd just replace a server fallback with the same client fallback. This would thrash the DOM, restart animations, etc. Since there is no reason to do this, in this PR, we instead throw away the commit and leave the server fallback in place. Then, when we're able to render the content, we commit that clean render and report the server error.

If there is another root update though, fallback props could've changed. E.g. theme switch. In that case we recreate it. (Unless it is a transition — that's not urgent, so it can wait.)

This fixes the case originally discovered in #24229. I've also added new tests just for this case.

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

sizebot · 2022-03-31T21:06:18Z

Comparing: 4db3ff6...6cb9f18

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.30 kB	131.30 kB	+0.03%	41.96 kB	41.97 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	136.36 kB	136.36 kB	+0.01%	43.41 kB	43.42 kB
facebook-www/ReactDOM-prod.classic.js	=	432.72 kB	432.75 kB	=	79.59 kB	79.58 kB
facebook-www/ReactDOM-prod.modern.js	=	417.72 kB	417.75 kB	=	77.21 kB	77.21 kB
facebook-www/ReactDOMForked-prod.classic.js	=	432.72 kB	432.75 kB	=	79.59 kB	79.58 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 6cb9f18

acdlite

This makes sense to me. I'm a little suspicious of the renderDidSuspendDelayIfPossible call but there's already a known problem related to that, so I can take a look when I work on that fix.

gaearon · 2022-03-31T21:15:10Z

Let's get Seb to look at the changed tests since this call was his idea

sebmarkbage

It would be nice to have a test that tests just the simple case. The server errors, the client is suspends but no errors. The fallback doesn’t get replaced. The it unsuspends.

With the rationale spelled out that we don’t want to interrupt animations in the fallback.

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

gaearon · 2022-03-31T21:55:38Z

OK so I'll add tests for:

Server errors, client doesn't but suspends. Client doesn't remount fallback because that would be silly.
Server errors, client doesn't but suspends. Then new prop update making fallback a different color. Client remounts.

Sounds right @sebmarkbage ?

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

…24236) * Delay showing fallback if hydrating suspends * Fix up * Include all non-urgent lanes * Moar tests * Add test for transitions

…acebook#24236) * Delay showing fallback if hydrating suspends * Fix up * Include all non-urgent lanes * Moar tests * Add test for transitions

… if hydrating suspends)

gaearon · 2022-04-25T15:16:45Z

I reverted this in #24434.

…ating suspends) (#24434) * Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) * Use @GATE FIXME

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

Delay showing fallback if hydrating suspends

38b3038

facebook-github-bot added the CLA Signed label Mar 31, 2022

gaearon requested review from acdlite and sebmarkbage March 31, 2022 21:02

facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Mar 31, 2022

acdlite reviewed Mar 31, 2022

View reviewed changes

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

Fix up

a47c615

acdlite approved these changes Mar 31, 2022

View reviewed changes

sebmarkbage approved these changes Mar 31, 2022

View reviewed changes

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

Include all non-urgent lanes

6f96d0a

sebmarkbage approved these changes Apr 1, 2022

View reviewed changes

Moar tests

e6734cd

gaearon force-pushed the delay-fallback-hydration branch from da9f213 to e6734cd Compare April 1, 2022 01:09

gaearon changed the title ~~Delay showing fallback if hydrating suspends~~ Don't recreate the same fallback on the client if hydrating suspends Apr 1, 2022

Add test for transitions

6cb9f18

gaearon merged commit ebd7ff6 into facebook:main Apr 1, 2022

gaearon deleted the delay-fallback-hydration branch April 1, 2022 01:50

gaearon added a commit to gaearon/react that referenced this pull request Apr 25, 2022

Revert facebook#24236

ad6b422

gaearon mentioned this pull request Apr 25, 2022

Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) #24434

Merged

gaearon added a commit to gaearon/react that referenced this pull request Apr 25, 2022

Revert facebook#24236 (Don't recreate the same fallback on the client…

02e1722

… if hydrating suspends)

gaearon added a commit that referenced this pull request Apr 25, 2022

Revert #24236 (Don't recreate the same fallback on the client if hydr…

bd4784c

…ating suspends) (#24434) * Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) * Use @GATE FIXME

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't recreate the same fallback on the client if hydrating suspends #24236

Don't recreate the same fallback on the client if hydrating suspends #24236

gaearon commented Mar 31, 2022 •

edited

Loading

sizebot commented Mar 31, 2022 •

edited

Loading

acdlite left a comment •

edited

Loading

gaearon commented Mar 31, 2022

sebmarkbage left a comment

gaearon commented Mar 31, 2022 •

edited

Loading

gaearon commented Apr 25, 2022

Don't recreate the same fallback on the client if hydrating suspends #24236

Don't recreate the same fallback on the client if hydrating suspends #24236

Conversation

gaearon commented Mar 31, 2022 • edited Loading

sizebot commented Mar 31, 2022 • edited Loading

Critical size changes

Significant size changes

acdlite left a comment • edited Loading

Choose a reason for hiding this comment

gaearon commented Mar 31, 2022

sebmarkbage left a comment

Choose a reason for hiding this comment

gaearon commented Mar 31, 2022 • edited Loading

gaearon commented Apr 25, 2022

gaearon commented Mar 31, 2022 •

edited

Loading

sizebot commented Mar 31, 2022 •

edited

Loading

acdlite left a comment •

edited

Loading

gaearon commented Mar 31, 2022 •

edited

Loading