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

Jump to bottom

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

Merged
merged 2 commits into from
Apr 25, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
02e1722
Revert #24236 (Don't recreate the same fallback on the client if hydr…
gaearon Apr 25, 2022
d7a9ef3
Use @gate FIXME
gaearon Apr 25, 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
38 changes: 23 additions & 15 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,15 @@ describe('ReactDOMFizzServer', () => {
});

// We still can't render it on the client.
expect(Scheduler).toFlushAndYield([]);
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);

// 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(<div>Loading...</div>);

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

// The client rendered HTML is now in place.
expect(getVisibleChildren(container)).toEqual(
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -2542,25 +2550,25 @@ 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(
<div>
<span />
<span>Yay!</span>
Loading...
<span />
</div>,
);

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(
<div>
<span />
Expand Down
18 changes: 12 additions & 6 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 @@ -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 (
<div className="parent">
Expand All @@ -809,12 +809,15 @@ describe('ReactDOMServerHydration', () => {
</div>
);
}
// 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 (
<div className="parent">
Expand All @@ -831,8 +834,11 @@ describe('ReactDOMServerHydration', () => {
</div>
);
}
// 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.]",
]
`);
});
});

Expand Down
5 changes: 4 additions & 1 deletion 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 @@ -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');
Expand Down
1 change: 0 additions & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
1 change: 0 additions & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
3 changes: 3 additions & 0 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 @@ -459,6 +459,9 @@ export function includesOnlyNonUrgentLanes(lanes: Lanes) {
const UrgentLanes = SyncLane | InputContinuousLane | DefaultLane;
return (lanes & UrgentLanes) === NoLanes;
}
export function includesOnlyTransitions(lanes: Lanes) {
Copy link
Collaborator Author

@gaearon gaearon Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I left the new includesOnlyNonUrgentLanes in the codebase because it's used by the deferred value logic too now. So this just adds the previous version of this function back.

return (lanes & TransitionLanes) === lanes;
}

export function includesBlockingLane(root: FiberRoot, lanes: Lanes) {
if (
Expand Down
3 changes: 3 additions & 0 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 @@ -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 (
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 @@ -131,7 +131,7 @@ import {
pickArbitraryLane,
includesNonIdleWork,
includesOnlyRetries,
includesOnlyNonUrgentLanes,
includesOnlyTransitions,
includesBlockingLane,
includesExpiredLane,
getNextLanes,
Expand Down Expand Up @@ -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.
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 @@ -131,7 +131,7 @@ import {
pickArbitraryLane,
includesNonIdleWork,
includesOnlyRetries,
includesOnlyNonUrgentLanes,
includesOnlyTransitions,
includesBlockingLane,
includesExpiredLane,
getNextLanes,
Expand Down Expand Up @@ -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.
Expand Down