Skip to content

Commit

Permalink
Bugfix: Suspending in shell during discrete update (#25495)
Browse files Browse the repository at this point in the history
Fixes a bug that happens when you suspend in the shell (the part of the
tree that is not wrapped in a Suspense boundary) during a
discrete update.

There were two underyling issues. One was just a mistake:
RootDidNotComplete needs to be handled in both renderRootConcurrent and
renderRootSync, but it was only handled in renderRootConcurrent. I did
it this way because I thought this path was unreachable during a sync
update, but I neglected to consider that renderRootSync is sometimes
called for non-concurrent lanes, like when recovering from an error, or
patching up a mutation to an external store.

After I fixed that oversight, the other issue is that we intentionally
error if the shell suspends during a sync update. The idea was that you
should either wrap the tree in a Suspense boundary, or you should mark
the update as a transition to allow React to suspend.

However, this did not take into account selective hydration, which can
force a sync render before anything has even committed. There's no way
in that case to wrap the update in startTransition.

Our solution for now is to remove the error that happens when you
suspend in the shell during a sync update — even for discrete updates.

We will likely revisit this in the future. One appealing possibility is
to commit the whole root in an inert state, as if it were a hidden
Offscreen tree.

Co-authored-by: Sebastian Markbåge <sebastian@calyptus.eu>

Co-authored-by: Sebastian Markbåge <sebastian@calyptus.eu>
  • Loading branch information
acdlite and sebmarkbage authored Oct 17, 2022
1 parent 54f297a commit 9ecf84e
Show file tree
Hide file tree
Showing 6 changed files with 138 additions and 59 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1695,4 +1695,81 @@ describe('ReactDOMServerSelectiveHydration', () => {
expect(triggeredParent).toBe(false);
expect(triggeredChild).toBe(false);
});

it('can attempt sync hydration if suspended root is still concurrently rendering', async () => {
let suspend = false;
let resolve;
const promise = new Promise(resolvePromise => (resolve = resolvePromise));
function Child({text}) {
if (suspend) {
throw promise;
}
Scheduler.unstable_yieldValue(text);
return (
<span
onClick={e => {
e.preventDefault();
Scheduler.unstable_yieldValue('Clicked ' + text);
}}>
{text}
</span>
);
}

function App() {
Scheduler.unstable_yieldValue('App');
return (
<div>
<Child text="A" />
</div>
);
}

const finalHTML = ReactDOMServer.renderToString(<App />);

expect(Scheduler).toHaveYielded(['App', 'A']);

const container = document.createElement('div');
// We need this to be in the document since we'll dispatch events on it.
document.body.appendChild(container);

container.innerHTML = finalHTML;

const span = container.getElementsByTagName('span')[0];

// We suspend on the client.
suspend = true;

React.startTransition(() => {
ReactDOMClient.hydrateRoot(container, <App />);
});
expect(Scheduler).toFlushAndYieldThrough(['App']);

// This should attempt to synchronously hydrate the root, then pause
// because it still suspended
const result = dispatchClickEvent(span);
expect(Scheduler).toHaveYielded(['App']);
// The event should not have been cancelled because we didn't hydrate.
expect(result).toBe(true);

// Finish loading the data
await act(async () => {
suspend = false;
await resolve();
});

// The app should have successfully hydrated and rendered
if (
gate(
flags =>
flags.enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay,
)
) {
expect(Scheduler).toHaveYielded(['App', 'A']);
} else {
expect(Scheduler).toHaveYielded(['App', 'A', 'Clicked A']);
}

document.body.removeChild(container);
});
});
38 changes: 16 additions & 22 deletions packages/react-reconciler/src/ReactFiberThrow.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,13 @@ import {
includesSomeLane,
mergeLanes,
pickArbitraryLane,
includesSyncLane,
} from './ReactFiberLane.new';
import {
getIsHydrating,
markDidThrowWhileHydratingDEV,
queueHydrationError,
} from './ReactFiberHydrationContext.new';
import {ConcurrentRoot} from './ReactRootTags';

function createRootErrorUpdate(
fiber: Fiber,
Expand Down Expand Up @@ -421,32 +421,26 @@ function throwException(
// No boundary was found. Unless this is a sync update, this is OK.
// We can suspend and wait for more data to arrive.

if (!includesSyncLane(rootRenderLanes)) {
// This is not a sync update. Suspend. Since we're not activating a
// Suspense boundary, this will unwind all the way to the root without
// performing a second pass to render a fallback. (This is arguably how
// refresh transitions should work, too, since we're not going to commit
// the fallbacks anyway.)
if (root.tag === ConcurrentRoot) {
// In a concurrent root, suspending without a Suspense boundary is
// allowed. It will suspend indefinitely without committing.
//
// This case also applies to initial hydration.
// TODO: Should we have different behavior for discrete updates? What
// about flushSync? Maybe it should put the tree into an inert state,
// and potentially log a warning. Revisit this for a future release.
attachPingListener(root, wakeable, rootRenderLanes);
renderDidSuspendDelayIfPossible();
return;
} else {
// In a legacy root, suspending without a boundary is always an error.
const uncaughtSuspenseError = new Error(
'A component suspended while responding to synchronous input. This ' +
'will cause the UI to be replaced with a loading indicator. To ' +
'fix, updates that suspend should be wrapped ' +
'with startTransition.',
);
value = uncaughtSuspenseError;
}

// This is a sync/discrete update. We treat this case like an error
// because discrete renders are expected to produce a complete tree
// synchronously to maintain consistency with external state.
const uncaughtSuspenseError = new Error(
'A component suspended while responding to synchronous input. This ' +
'will cause the UI to be replaced with a loading indicator. To ' +
'fix, updates that suspend should be wrapped ' +
'with startTransition.',
);

// If we're outside a transition, fall through to the regular error path.
// The error will be caught by the nearest suspense boundary.
value = uncaughtSuspenseError;
}
} else {
// This is a regular error, not a Suspense wakeable.
Expand Down
38 changes: 16 additions & 22 deletions packages/react-reconciler/src/ReactFiberThrow.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,13 @@ import {
includesSomeLane,
mergeLanes,
pickArbitraryLane,
includesSyncLane,
} from './ReactFiberLane.old';
import {
getIsHydrating,
markDidThrowWhileHydratingDEV,
queueHydrationError,
} from './ReactFiberHydrationContext.old';
import {ConcurrentRoot} from './ReactRootTags';

function createRootErrorUpdate(
fiber: Fiber,
Expand Down Expand Up @@ -421,32 +421,26 @@ function throwException(
// No boundary was found. Unless this is a sync update, this is OK.
// We can suspend and wait for more data to arrive.

if (!includesSyncLane(rootRenderLanes)) {
// This is not a sync update. Suspend. Since we're not activating a
// Suspense boundary, this will unwind all the way to the root without
// performing a second pass to render a fallback. (This is arguably how
// refresh transitions should work, too, since we're not going to commit
// the fallbacks anyway.)
if (root.tag === ConcurrentRoot) {
// In a concurrent root, suspending without a Suspense boundary is
// allowed. It will suspend indefinitely without committing.
//
// This case also applies to initial hydration.
// TODO: Should we have different behavior for discrete updates? What
// about flushSync? Maybe it should put the tree into an inert state,
// and potentially log a warning. Revisit this for a future release.
attachPingListener(root, wakeable, rootRenderLanes);
renderDidSuspendDelayIfPossible();
return;
} else {
// In a legacy root, suspending without a boundary is always an error.
const uncaughtSuspenseError = new Error(
'A component suspended while responding to synchronous input. This ' +
'will cause the UI to be replaced with a loading indicator. To ' +
'fix, updates that suspend should be wrapped ' +
'with startTransition.',
);
value = uncaughtSuspenseError;
}

// This is a sync/discrete update. We treat this case like an error
// because discrete renders are expected to produce a complete tree
// synchronously to maintain consistency with external state.
const uncaughtSuspenseError = new Error(
'A component suspended while responding to synchronous input. This ' +
'will cause the UI to be replaced with a loading indicator. To ' +
'fix, updates that suspend should be wrapped ' +
'with startTransition.',
);

// If we're outside a transition, fall through to the regular error path.
// The error will be caught by the nearest suspense boundary.
value = uncaughtSuspenseError;
}
} else {
// This is a regular error, not a Suspense wakeable.
Expand Down
14 changes: 9 additions & 5 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -1063,10 +1063,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
// The render unwound without completing the tree. This happens in special
// cases where need to exit the current render without producing a
// consistent tree or committing.
//
// This should only happen during a concurrent render, not a discrete or
// synchronous update. We should have already checked for this when we
// unwound the stack.
markRootSuspended(root, lanes);
} else {
// The render completed.
Expand Down Expand Up @@ -1111,6 +1107,9 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
ensureRootIsScheduled(root, now());
throw fatalError;
}

// FIXME: Need to check for RootDidNotComplete again. The factoring here
// isn't ideal.
}

// We now have a consistent tree. The next step is either to commit it,
Expand Down Expand Up @@ -1473,7 +1472,12 @@ function performSyncWorkOnRoot(root) {
}

if (exitStatus === RootDidNotComplete) {
throw new Error('Root did not complete. This is a bug in React.');
// The render unwound without completing the tree. This happens in special
// cases where need to exit the current render without producing a
// consistent tree or committing.
markRootSuspended(root, lanes);
ensureRootIsScheduled(root, now());
return null;
}

// We now have a consistent tree. Because this is a sync render, we
Expand Down
14 changes: 9 additions & 5 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -1063,10 +1063,6 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
// The render unwound without completing the tree. This happens in special
// cases where need to exit the current render without producing a
// consistent tree or committing.
//
// This should only happen during a concurrent render, not a discrete or
// synchronous update. We should have already checked for this when we
// unwound the stack.
markRootSuspended(root, lanes);
} else {
// The render completed.
Expand Down Expand Up @@ -1111,6 +1107,9 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
ensureRootIsScheduled(root, now());
throw fatalError;
}

// FIXME: Need to check for RootDidNotComplete again. The factoring here
// isn't ideal.
}

// We now have a consistent tree. The next step is either to commit it,
Expand Down Expand Up @@ -1473,7 +1472,12 @@ function performSyncWorkOnRoot(root) {
}

if (exitStatus === RootDidNotComplete) {
throw new Error('Root did not complete. This is a bug in React.');
// The render unwound without completing the tree. This happens in special
// cases where need to exit the current render without producing a
// consistent tree or committing.
markRootSuspended(root, lanes);
ensureRootIsScheduled(root, now());
return null;
}

// We now have a consistent tree. Because this is a sync render, we
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1012,15 +1012,21 @@ describe('ReactSuspenseWithNoopRenderer', () => {
});

// @gate enableCache
it('errors when an update suspends without a placeholder during a sync update', () => {
// This is an error because sync/discrete updates are expected to produce
// a complete tree immediately to maintain consistency with external state
// — we can't delay the commit.
it('in concurrent mode, does not error when an update suspends without a Suspense boundary during a sync update', () => {
// NOTE: We may change this to be a warning in the future.
expect(() => {
ReactNoop.flushSync(() => {
ReactNoop.render(<AsyncText text="Async" />);
});
}).toThrow('A component suspended while responding to synchronous input.');
}).not.toThrow();
});

// @gate enableCache
it('in legacy mode, errors when an update suspends without a Suspense boundary during a sync update', () => {
const root = ReactNoop.createLegacyRoot();
expect(() => root.render(<AsyncText text="Async" />)).toThrow(
'A component suspended while responding to synchronous input.',
);
});

// @gate enableCache
Expand Down

0 comments on commit 9ecf84e

Please sign in to comment.