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.)
Andrew Clark committed
Mar 20, 2023 at 22:07 UTC
77ba1618a528787baaa8e007fadaff93a86fb675
2 files changed
+28
-6
packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
+26
-2
@@ -19,6 +19,7 @@ let ReactFeatureFlags;
19
let Suspense;
20
let SuspenseList;
21
let Offscreen;
22
+let useSyncExternalStore;
23
let act;
24
let IdleEventPriority;
25
let waitForAll;
@@ -113,6 +114,7 @@ describe('ReactDOMServerPartialHydration', () => {
114
Scheduler = require('scheduler');
115
Suspense = React.Suspense;
116
Offscreen = React.unstable_Offscreen;
117
+ useSyncExternalStore = React.useSyncExternalStore;
118
if (gate(flags => flags.enableSuspenseList)) {
119
SuspenseList = React.SuspenseList;
120
}
@@ -480,6 +482,26 @@ describe('ReactDOMServerPartialHydration', () => {
482
});
483
484
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
+
505
const ref = React.createRef();
506
function App({hasB}) {
507
return (
@@ -487,6 +509,7 @@ describe('ReactDOMServerPartialHydration', () => {
509
<Suspense fallback="Loading...">
510
<span ref={ref}>A</span>
511
{hasB ? <span>B</span> : null}
512
+ <CheckIfHydrating />
513
</Suspense>
514
<div>Sibling</div>
515
</div>
@@ -494,6 +517,7 @@ describe('ReactDOMServerPartialHydration', () => {
517
}
518
519
const finalHTML = ReactDOMServer.renderToString(<App hasB={true} />);
520
+ assertLog(['Server rendered']);
521
522
const container = document.createElement('div');
523
container.innerHTML = finalHTML;
@@ -514,12 +538,12 @@ describe('ReactDOMServerPartialHydration', () => {
538
});
539
}).toErrorDev('Did not expect server HTML to contain a <span> in <div>');
540
517
- jest.runAllTimers();
518
-
541
expect(container.innerHTML).toContain('<span>A</span>');
542
expect(container.innerHTML).not.toContain('<span>B</span>');
543
544
assertLog([
545
+ 'Server rendered',
546
+ 'Client rendered',
547
'There was an error while hydrating this Suspense boundary. ' +
548
'Switched to client rendering.',
549
]);
packages/react-reconciler/src/ReactFiberCompleteWork.js
+2
-4
@@ -85,8 +85,6 @@ import {
85
StaticMask,
86
MutationMask,
87
Passive,
88
- Incomplete,
89
- ShouldCapture,
88
ForceClientRender,
89
SuspenseyCommit,
90
ScheduleRetry,
@@ -839,7 +837,7 @@ function completeDehydratedSuspenseBoundary(
837
) {
838
warnIfUnhydratedTailNodes(workInProgress);
839
resetHydrationState();
842
- workInProgress.flags |= ForceClientRender | Incomplete | ShouldCapture;
840
+ workInProgress.flags |= ForceClientRender | DidCapture;
841
842
return false;
843
}
@@ -1284,7 +1282,7 @@ function completeWork(
1282
nextState,
1283
);
1284
if (!fallthroughToNormalSuspensePath) {
1287
- if (workInProgress.flags & ShouldCapture) {
1285
+ if (workInProgress.flags & ForceClientRender) {
1286
// Special case. There were remaining unhydrated nodes. We treat
1287
// this as a mismatch. Revert to client rendering.
1288
return workInProgress;