Skip to content

Commit 77ba161

Browse files
authored
Bugfix: Remove extra render pass when reverting to client render (#26445)
(This was reviewed and approved as part of #26380; I'm extracting it into its own PR so that it can bisected later if it causes an issue.) I noticed while working on a PR that when an error happens during hydration, and we revert to client rendering, React actually does _two_ additional render passes instead of just one. We didn't notice it earlier because none of our tests happened to assert on how many renders it took to recover, only on the final output. It's possible this extra render pass had other consequences that I'm not aware of, like messing with some assumption in the recoverable errors logic. This adds a test to demonstrate the issue. (One problem is that we don't have much test coverage of this scenario in the first place, which likely would have caught this earlier.)
1 parent 520f7f3 commit 77ba161

File tree

2 files changed

+28
-6
lines changed

2 files changed

+28
-6
lines changed

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

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ let ReactFeatureFlags;
1919
let Suspense;
2020
let SuspenseList;
2121
let Offscreen;
22+
let useSyncExternalStore;
2223
let act;
2324
let IdleEventPriority;
2425
let waitForAll;
@@ -113,6 +114,7 @@ describe('ReactDOMServerPartialHydration', () => {
113114
Scheduler = require('scheduler');
114115
Suspense = React.Suspense;
115116
Offscreen = React.unstable_Offscreen;
117+
useSyncExternalStore = React.useSyncExternalStore;
116118
if (gate(flags => flags.enableSuspenseList)) {
117119
SuspenseList = React.SuspenseList;
118120
}
@@ -480,20 +482,42 @@ describe('ReactDOMServerPartialHydration', () => {
480482
});
481483

482484
it('recovers with client render when server rendered additional nodes at suspense root', async () => {
485+
function CheckIfHydrating({children}) {
486+
// This is a trick to check whether we're hydrating or not, since React
487+
// doesn't expose that information currently except
488+
// via useSyncExternalStore.
489+
let serverOrClient = '(unknown)';
490+
useSyncExternalStore(
491+
() => {},
492+
() => {
493+
serverOrClient = 'Client rendered';
494+
return null;
495+
},
496+
() => {
497+
serverOrClient = 'Server rendered';
498+
return null;
499+
},
500+
);
501+
Scheduler.log(serverOrClient);
502+
return null;
503+
}
504+
483505
const ref = React.createRef();
484506
function App({hasB}) {
485507
return (
486508
<div>
487509
<Suspense fallback="Loading...">
488510
<span ref={ref}>A</span>
489511
{hasB ? <span>B</span> : null}
512+
<CheckIfHydrating />
490513
</Suspense>
491514
<div>Sibling</div>
492515
</div>
493516
);
494517
}
495518

496519
const finalHTML = ReactDOMServer.renderToString(<App hasB={true} />);
520+
assertLog(['Server rendered']);
497521

498522
const container = document.createElement('div');
499523
container.innerHTML = finalHTML;
@@ -514,12 +538,12 @@ describe('ReactDOMServerPartialHydration', () => {
514538
});
515539
}).toErrorDev('Did not expect server HTML to contain a <span> in <div>');
516540

517-
jest.runAllTimers();
518-
519541
expect(container.innerHTML).toContain('<span>A</span>');
520542
expect(container.innerHTML).not.toContain('<span>B</span>');
521543

522544
assertLog([
545+
'Server rendered',
546+
'Client rendered',
523547
'There was an error while hydrating this Suspense boundary. ' +
524548
'Switched to client rendering.',
525549
]);

packages/react-reconciler/src/ReactFiberCompleteWork.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -85,8 +85,6 @@ import {
8585
StaticMask,
8686
MutationMask,
8787
Passive,
88-
Incomplete,
89-
ShouldCapture,
9088
ForceClientRender,
9189
SuspenseyCommit,
9290
ScheduleRetry,
@@ -839,7 +837,7 @@ function completeDehydratedSuspenseBoundary(
839837
) {
840838
warnIfUnhydratedTailNodes(workInProgress);
841839
resetHydrationState();
842-
workInProgress.flags |= ForceClientRender | Incomplete | ShouldCapture;
840+
workInProgress.flags |= ForceClientRender | DidCapture;
843841

844842
return false;
845843
}
@@ -1284,7 +1282,7 @@ function completeWork(
12841282
nextState,
12851283
);
12861284
if (!fallthroughToNormalSuspensePath) {
1287-
if (workInProgress.flags & ShouldCapture) {
1285+
if (workInProgress.flags & ForceClientRender) {
12881286
// Special case. There were remaining unhydrated nodes. We treat
12891287
// this as a mismatch. Revert to client rendering.
12901288
return workInProgress;

0 commit comments

Comments
 (0)