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.

Sign up for GitHub

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 recreate the same fallback on the client if hydrating suspends #24236

Merged
merged 5 commits into from
Apr 1, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
38b3038
Delay showing fallback if hydrating suspends
gaearon Mar 31, 2022
a47c615
Fix up
gaearon Mar 31, 2022
6f96d0a
Include all non-urgent lanes
gaearon Mar 31, 2022
e6734cd
Moar tests
gaearon Apr 1, 2022
6cb9f18
Add test for transitions
gaearon Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,16 +869,16 @@ describe('ReactDOMFizzServer', () => {
});

// We still can't render it on the client.
expect(Scheduler).toFlushAndYield([
'The server could not finish this Suspense boundary, likely due to an ' +
'error during server rendering. Switched to client rendering.',
]);
expect(Scheduler).toFlushAndYield([]);
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

// We now resolve it on the client.
resolveText('Hello');

Scheduler.unstable_flushAll();
expect(Scheduler).toFlushAndYield([
'The server could not finish this Suspense boundary, likely due to an ' +
'error during server rendering. Switched to client rendering.',
]);

// The client rendered HTML is now in place.
expect(getVisibleChildren(container)).toEqual(
Expand Down Expand Up @@ -2293,25 +2293,25 @@ describe('ReactDOMFizzServer', () => {
},
});

// 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.',
]);
// An error happened but instead of surfacing it to the UI, we suspended.
expect(Scheduler).toFlushAndYield([]);
expect(getVisibleChildren(container)).toEqual(
<div>
<span />
Loading...
<span>Yay!</span>
<span />
</div>,
);

await act(async () => {
resolveText('Yay!');
});
expect(Scheduler).toFlushAndYield(['Yay!']);
expect(Scheduler).toFlushAndYield([
'Yay!',
'Hydration error',
'There was an error while hydrating this Suspense boundary. Switched ' +
'to client rendering.',
]);
expect(getVisibleChildren(container)).toEqual(
<div>
<span />
Expand Down
34 changes: 8 additions & 26 deletions packages/react-dom/src/__tests__/ReactDOMHydrationDiff-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1046,7 +1046,7 @@ describe('ReactDOMServerHydration', () => {
});

// @gate __DEV__
it('warns when client renders an extra node inside Suspense fallback', () => {
it('does not warn when client renders an extra node inside Suspense fallback', () => {
function Mismatch({isClient}) {
return (
<div className="parent">
Expand All @@ -1063,27 +1063,18 @@ describe('ReactDOMServerHydration', () => {
</div>
);
}
// TODO: Why does this not show a fallback mismatch?
// And why is this message different from the other ones?
if (
gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)
) {
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.]",
]
`);
// There is no error because we don't actually hydrate fallbacks.
gaearon marked this conversation as resolved.
Show resolved Hide resolved
expect(testMismatch(Mismatch)).toMatchInlineSnapshot(`Array []`);
} else {
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.]",
]
`);
expect(testMismatch(Mismatch)).toMatchInlineSnapshot(`Array []`);
}
});

// @gate __DEV__
it('warns when server renders an extra node inside Suspense fallback', () => {
it('does not warn when server renders an extra node inside Suspense fallback', () => {
function Mismatch({isClient}) {
return (
<div className="parent">
Expand All @@ -1100,22 +1091,13 @@ describe('ReactDOMServerHydration', () => {
</div>
);
}
// TODO: Why does this not show a fallback mismatch?
// And why is this message different from the other ones?
if (
gate(flags => flags.enableClientRenderFallbackOnHydrationMismatch)
) {
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.]",
]
`);
// There is no error because we don't actually hydrate fallbacks.
expect(testMismatch(Mismatch)).toMatchInlineSnapshot(`Array []`);
} else {
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.]",
]
`);
expect(testMismatch(Mismatch)).toMatchInlineSnapshot(`Array []`);
}
});
});
Expand Down
5 changes: 1 addition & 4 deletions packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2137,10 +2137,7 @@ describe('ReactDOMServerPartialHydration', () => {
});

suspend = true;
expect(Scheduler).toFlushAndYield([
'The server could not finish this Suspense boundary, likely due to ' +
'an error during server rendering. Switched to client rendering.',
]);
expect(Scheduler).toFlushAndYield([]);

// We haven't hydrated the second child but the placeholder is still in the list.
expect(container.textContent).toBe('ALoading B');
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2241,6 +2241,7 @@ 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(
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2241,6 +2241,7 @@ 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(
Expand Down
11 changes: 9 additions & 2 deletions packages/react-reconciler/src/ReactFiberLane.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,15 @@ export function includesNonIdleWork(lanes: Lanes) {
export function includesOnlyRetries(lanes: Lanes) {
return (lanes & RetryLanes) === lanes;
}
export function includesOnlyTransitions(lanes: Lanes) {
return (lanes & TransitionLanes) === lanes;
export function includesOnlyTransitionsOrHydration(lanes: Lanes) {
const TransitionOrHydrationLanes =
TransitionLanes |
InputContinuousHydrationLane |
DefaultHydrationLane |
TransitionHydrationLane |
SelectiveHydrationLane |
IdleHydrationLane;
return (lanes & TransitionOrHydrationLanes) === lanes;
}

export function includesBlockingLane(root: FiberRoot, lanes: Lanes) {
Expand Down
11 changes: 9 additions & 2 deletions packages/react-reconciler/src/ReactFiberLane.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,15 @@ export function includesNonIdleWork(lanes: Lanes) {
export function includesOnlyRetries(lanes: Lanes) {
return (lanes & RetryLanes) === lanes;
}
export function includesOnlyTransitions(lanes: Lanes) {
return (lanes & TransitionLanes) === lanes;
export function includesOnlyTransitionsOrHydration(lanes: Lanes) {
const TransitionOrHydrationLanes =
TransitionLanes |
InputContinuousHydrationLane |
DefaultHydrationLane |
TransitionHydrationLane |
SelectiveHydrationLane |
IdleHydrationLane;
return (lanes & TransitionOrHydrationLanes) === lanes;
}

export function includesBlockingLane(root: FiberRoot, lanes: Lanes) {
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ import {
pickArbitraryLane,
includesNonIdleWork,
includesOnlyRetries,
includesOnlyTransitions,
includesOnlyTransitionsOrHydration,
includesBlockingLane,
includesExpiredLane,
getNextLanes,
Expand Down Expand Up @@ -1110,7 +1110,7 @@ function finishConcurrentRender(root, exitStatus, lanes) {
case RootSuspendedWithDelay: {
markRootSuspended(root, lanes);

if (includesOnlyTransitions(lanes)) {
if (includesOnlyTransitionsOrHydration(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.
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ import {
pickArbitraryLane,
includesNonIdleWork,
includesOnlyRetries,
includesOnlyTransitions,
includesOnlyTransitionsOrHydration,
includesBlockingLane,
includesExpiredLane,
getNextLanes,
Expand Down Expand Up @@ -1110,7 +1110,7 @@ function finishConcurrentRender(root, exitStatus, lanes) {
case RootSuspendedWithDelay: {
markRootSuspended(root, lanes);

if (includesOnlyTransitions(lanes)) {
if (includesOnlyTransitionsOrHydration(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.
Expand Down