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

Bugfix: Suspending in shell during discrete update #25495

Merged
merged 1 commit into from
Oct 17, 2022
Merged

Bugfix: Suspending in shell during discrete update #25495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
77 changes: 77 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.
Comment on lines +1110 to +1112
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume that you intentionally left this to a later fix and did not intend to address it now (flagging in case that's not true)

}

// 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
View file
Open in desktop
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
16 changes: 11 additions & 5 deletions packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
View file
Open in desktop
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