From bd4784c8f8c6b17cf45c712db8ed8ed19a622b26 Mon Sep 17 00:00:00 2001 From: dan Date: Mon, 25 Apr 2022 16:16:32 +0100 Subject: [PATCH] Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) (#24434) * Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) * Use @gate FIXME --- .../src/__tests__/ReactDOMFizzServer-test.js | 38 +++++++++++-------- .../__tests__/ReactDOMHydrationDiff-test.js | 18 ++++++--- ...DOMServerPartialHydration-test.internal.js | 5 ++- .../src/ReactFiberBeginWork.new.js | 1 - .../src/ReactFiberBeginWork.old.js | 1 - .../src/ReactFiberLane.new.js | 3 ++ .../src/ReactFiberLane.old.js | 3 ++ .../src/ReactFiberWorkLoop.new.js | 4 +- .../src/ReactFiberWorkLoop.old.js | 4 +- 9 files changed, 49 insertions(+), 28 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index bf5388597571e..72ed9f8775dab 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -869,16 +869,15 @@ describe('ReactDOMFizzServer', () => { }); // We still can't render it on the client. - expect(Scheduler).toFlushAndYield([]); - expect(getVisibleChildren(container)).toEqual(
Loading...
); - - // We now resolve it on the client. - resolveText('Hello'); - expect(Scheduler).toFlushAndYield([ 'The server could not finish this Suspense boundary, likely due to an ' + 'error during server rendering. Switched to client rendering.', ]); + expect(getVisibleChildren(container)).toEqual(
Loading...
); + + // We now resolve it on the client. + resolveText('Hello'); + Scheduler.unstable_flushAll(); // The client rendered HTML is now in place. expect(getVisibleChildren(container)).toEqual( @@ -2189,7 +2188,10 @@ describe('ReactDOMFizzServer', () => { }, ); + // Disabled because of a WWW late mutations regression. + // We may want to re-enable this if we figure out why. // @gate experimental + // @gate FIXME it('does not recreate the fallback if server errors and hydration suspends', async () => { let isClient = false; @@ -2268,7 +2270,10 @@ describe('ReactDOMFizzServer', () => { ); }); + // Disabled because of a WWW late mutations regression. + // We may want to re-enable this if we figure out why. // @gate experimental + // @gate FIXME it( 'does not recreate the fallback if server errors and hydration suspends ' + 'and root receives a transition', @@ -2364,7 +2369,10 @@ describe('ReactDOMFizzServer', () => { }, ); + // Disabled because of a WWW late mutations regression. + // We may want to re-enable this if we figure out why. // @gate experimental + // @gate FIXME it( 'recreates the fallback if server errors and hydration suspends but ' + 'client receives new props', @@ -2542,12 +2550,17 @@ describe('ReactDOMFizzServer', () => { }, }); - // An error happened but instead of surfacing it to the UI, we suspended. - expect(Scheduler).toFlushAndYield([]); + // An error logged but instead of surfacing it to the UI, we switched + // to client rendering. + expect(Scheduler).toFlushAndYield([ + 'Hydration error', + 'There was an error while hydrating this Suspense boundary. Switched ' + + 'to client rendering.', + ]); expect(getVisibleChildren(container)).toEqual(
- Yay! + Loading...
, ); @@ -2555,12 +2568,7 @@ describe('ReactDOMFizzServer', () => { await act(async () => { resolveText('Yay!'); }); - expect(Scheduler).toFlushAndYield([ - 'Yay!', - 'Hydration error', - 'There was an error while hydrating this Suspense boundary. Switched ' + - 'to client rendering.', - ]); + expect(Scheduler).toFlushAndYield(['Yay!']); expect(getVisibleChildren(container)).toEqual(
diff --git a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js index 9237493c6dcec..f6b5a0b428a7d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js @@ -792,7 +792,7 @@ describe('ReactDOMServerHydration', () => { }); // @gate __DEV__ - it('does not warn when client renders an extra node inside Suspense fallback', () => { + it('warns when client renders an extra node inside Suspense fallback', () => { function Mismatch({isClient}) { return (
@@ -809,12 +809,15 @@ describe('ReactDOMServerHydration', () => {
); } - // There is no error because we don't actually hydrate fallbacks. - expect(testMismatch(Mismatch)).toMatchInlineSnapshot(`Array []`); + expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` + Array [ + "Caught [The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.]", + ] + `); }); // @gate __DEV__ - it('does not warn when server renders an extra node inside Suspense fallback', () => { + it('warns when server renders an extra node inside Suspense fallback', () => { function Mismatch({isClient}) { return (
@@ -831,8 +834,11 @@ describe('ReactDOMServerHydration', () => {
); } - // There is no error because we don't actually hydrate fallbacks. - expect(testMismatch(Mismatch)).toMatchInlineSnapshot(`Array []`); + expect(testMismatch(Mismatch)).toMatchInlineSnapshot(` + Array [ + "Caught [The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.]", + ] + `); }); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index fe9d8dd453cdb..e4d33645820e9 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -2115,7 +2115,10 @@ describe('ReactDOMServerPartialHydration', () => { }); suspend = true; - expect(Scheduler).toFlushAndYield([]); + expect(Scheduler).toFlushAndYield([ + 'The server could not finish this Suspense boundary, likely due to ' + + 'an error during server rendering. Switched to client rendering.', + ]); // We haven't hydrated the second child but the placeholder is still in the list. expect(container.textContent).toBe('ALoading B'); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 0764f16c89a0a..1538e32fca272 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -2270,7 +2270,6 @@ function updateSuspenseComponent(current, workInProgress, renderLanes) { } else { // Suspended but we should no longer be in dehydrated mode. // Therefore we now have to render the fallback. - renderDidSuspendDelayIfPossible(); const nextPrimaryChildren = nextProps.children; const nextFallbackChildren = nextProps.fallback; const fallbackChildFragment = mountSuspenseFallbackAfterRetryWithoutHydrating( diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js index 6d6e15e24af61..c50ca6357a5d9 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js @@ -2270,7 +2270,6 @@ function updateSuspenseComponent(current, workInProgress, renderLanes) { } else { // Suspended but we should no longer be in dehydrated mode. // Therefore we now have to render the fallback. - renderDidSuspendDelayIfPossible(); const nextPrimaryChildren = nextProps.children; const nextFallbackChildren = nextProps.fallback; const fallbackChildFragment = mountSuspenseFallbackAfterRetryWithoutHydrating( diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index aa3360bd3c2b9..708c9318b3a98 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -459,6 +459,9 @@ export function includesOnlyNonUrgentLanes(lanes: Lanes) { const UrgentLanes = SyncLane | InputContinuousLane | DefaultLane; return (lanes & UrgentLanes) === NoLanes; } +export function includesOnlyTransitions(lanes: Lanes) { + return (lanes & TransitionLanes) === lanes; +} export function includesBlockingLane(root: FiberRoot, lanes: Lanes) { if ( diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js index a65a922d99b9a..e71aa5575abb6 100644 --- a/packages/react-reconciler/src/ReactFiberLane.old.js +++ b/packages/react-reconciler/src/ReactFiberLane.old.js @@ -459,6 +459,9 @@ export function includesOnlyNonUrgentLanes(lanes: Lanes) { const UrgentLanes = SyncLane | InputContinuousLane | DefaultLane; return (lanes & UrgentLanes) === NoLanes; } +export function includesOnlyTransitions(lanes: Lanes) { + return (lanes & TransitionLanes) === lanes; +} export function includesBlockingLane(root: FiberRoot, lanes: Lanes) { if ( diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index aa20335cbec91..89c880d868554 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -131,7 +131,7 @@ import { pickArbitraryLane, includesNonIdleWork, includesOnlyRetries, - includesOnlyNonUrgentLanes, + includesOnlyTransitions, includesBlockingLane, includesExpiredLane, getNextLanes, @@ -1150,7 +1150,7 @@ function finishConcurrentRender(root, exitStatus, lanes) { case RootSuspendedWithDelay: { markRootSuspended(root, lanes); - if (includesOnlyNonUrgentLanes(lanes)) { + if (includesOnlyTransitions(lanes)) { // This is a transition, so we should exit without committing a // placeholder and without scheduling a timeout. Delay indefinitely // until we receive more data. diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index e2c3a76c7fc24..2f5dbe8530ce5 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -131,7 +131,7 @@ import { pickArbitraryLane, includesNonIdleWork, includesOnlyRetries, - includesOnlyNonUrgentLanes, + includesOnlyTransitions, includesBlockingLane, includesExpiredLane, getNextLanes, @@ -1150,7 +1150,7 @@ function finishConcurrentRender(root, exitStatus, lanes) { case RootSuspendedWithDelay: { markRootSuspended(root, lanes); - if (includesOnlyNonUrgentLanes(lanes)) { + if (includesOnlyTransitions(lanes)) { // This is a transition, so we should exit without committing a // placeholder and without scheduling a timeout. Delay indefinitely // until we receive more data.