Skip to content

Commit

Permalink
Fix: Suspend while recovering from hydration error (#28800)
Browse files Browse the repository at this point in the history
Fixes a bug that happens when an error occurs during hydration, React
switches to client rendering, and then the client render suspends. It
works correctly if there's a Suspense boundary on the stack, but not if
it happens in the shell of the app.

Prior to this fix, the app would crash with an "Unknown root exit
status" error.

I left a TODO comment for how we might refactor this code to be less
confusing in the future.
  • Loading branch information
acdlite committed Apr 9, 2024
1 parent 64c8d2d commit 3f9e237
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -548,4 +548,79 @@ describe('ReactDOMFizzShellHydration', () => {
]);
expect(container.textContent).toBe('Hello world');
});

it(
'handles suspending while recovering from a hydration error (in the ' +
'shell, no Suspense boundary)',
async () => {
const useSyncExternalStore = React.useSyncExternalStore;

let isClient = false;

let resolve;
const clientPromise = new Promise(res => {
resolve = res;
});

function App() {
const state = useSyncExternalStore(
function subscribe() {
return () => {};
},
function getSnapshot() {
return 'Client';
},
function getServerSnapshot() {
const isHydrating = isClient;
if (isHydrating) {
// This triggers an error during hydration
throw new Error('Oops!');
}
return 'Server';
},
);

if (state === 'Client') {
return React.use(clientPromise);
}

return state;
}

// Server render
await serverAct(async () => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
pipe(writable);
});
assertLog([]);

expect(container.innerHTML).toBe('Server');

// During hydration, an error is thrown. React attempts to recover by
// switching to client render
isClient = true;
await clientAct(async () => {
ReactDOMClient.hydrateRoot(container, <App />, {
onRecoverableError(error) {
Scheduler.log('onRecoverableError: ' + error.message);
if (error.cause) {
Scheduler.log('Cause: ' + error.cause.message);
}
},
});
});
expect(container.innerHTML).toBe('Server'); // Still suspended
assertLog([]);

await clientAct(async () => {
resolve('Client');
});
assertLog([
'onRecoverableError: There was an error while hydrating but React was ' +
'able to recover by instead client rendering the entire root.',
'Cause: Oops!',
]);
expect(container.innerHTML).toBe('Client');
},
);
});
19 changes: 16 additions & 3 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -931,19 +931,32 @@ export function performConcurrentWorkOnRoot(

// Check if something threw
if (exitStatus === RootErrored) {
const originallyAttemptedLanes = lanes;
const lanesThatJustErrored = lanes;
const errorRetryLanes = getLanesToRetrySynchronouslyOnError(
root,
originallyAttemptedLanes,
lanesThatJustErrored,
);
if (errorRetryLanes !== NoLanes) {
lanes = errorRetryLanes;
exitStatus = recoverFromConcurrentError(
root,
originallyAttemptedLanes,
lanesThatJustErrored,
errorRetryLanes,
);
renderWasConcurrent = false;
// Need to check the exit status again.
if (exitStatus !== RootErrored) {
// The root did not error this time. Restart the exit algorithm
// from the beginning.
// TODO: Refactor the exit algorithm to be less confusing. Maybe
// more branches + recursion instead of a loop. I think the only
// thing that causes it to be a loop is the RootDidNotComplete
// check. If that's true, then we don't need a loop/recursion
// at all.
continue;
} else {
// The root errored yet again. Proceed to commit the tree.
}
}
}
if (exitStatus === RootFatalErrored) {
Expand Down

0 comments on commit 3f9e237

Please sign in to comment.