diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
index 59adc4c1b5ad0..9bc53e4f30e78 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -1849,7 +1849,7 @@ function handleThrow(root, thrownValue): void {
function shouldAttemptToSuspendUntilDataResolves() {
// TODO: We should be able to move the
- // renderDidSuspend/renderDidSuspendWithDelay logic into this function,
+ // renderDidSuspend/renderDidSuspendDelayIfPossible logic into this function,
// instead of repeating it in the complete phase. Or something to that effect.
if (includesOnlyRetries(workInProgressRootRenderLanes)) {
@@ -1857,6 +1857,18 @@ function shouldAttemptToSuspendUntilDataResolves() {
return true;
}
+ // Check if there are other pending updates that might possibly unblock this
+ // component from suspending. This mirrors the check in
+ // renderDidSuspendDelayIfPossible. We should attempt to unify them somehow.
+ if (
+ includesNonIdleWork(workInProgressRootSkippedLanes) ||
+ includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes)
+ ) {
+ // Suspend normally. renderDidSuspendDelayIfPossible will handle
+ // interrupting the work loop.
+ return false;
+ }
+
// TODO: We should be able to remove the equivalent check in
// finishConcurrentRender, and rely just on this one.
if (includesOnlyTransitions(workInProgressRootRenderLanes)) {
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
index 101004007e399..12293f0210e0f 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -1849,7 +1849,7 @@ function handleThrow(root, thrownValue): void {
function shouldAttemptToSuspendUntilDataResolves() {
// TODO: We should be able to move the
- // renderDidSuspend/renderDidSuspendWithDelay logic into this function,
+ // renderDidSuspend/renderDidSuspendDelayIfPossible logic into this function,
// instead of repeating it in the complete phase. Or something to that effect.
if (includesOnlyRetries(workInProgressRootRenderLanes)) {
@@ -1857,6 +1857,18 @@ function shouldAttemptToSuspendUntilDataResolves() {
return true;
}
+ // Check if there are other pending updates that might possibly unblock this
+ // component from suspending. This mirrors the check in
+ // renderDidSuspendDelayIfPossible. We should attempt to unify them somehow.
+ if (
+ includesNonIdleWork(workInProgressRootSkippedLanes) ||
+ includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes)
+ ) {
+ // Suspend normally. renderDidSuspendDelayIfPossible will handle
+ // interrupting the work loop.
+ return false;
+ }
+
// TODO: We should be able to remove the equivalent check in
// finishConcurrentRender, and rely just on this one.
if (includesOnlyTransitions(workInProgressRootRenderLanes)) {
diff --git a/packages/react-reconciler/src/__tests__/ReactThenable-test.js b/packages/react-reconciler/src/__tests__/ReactThenable-test.js
index f0bca1420145c..06a11a2101225 100644
--- a/packages/react-reconciler/src/__tests__/ReactThenable-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactThenable-test.js
@@ -5,6 +5,7 @@ let ReactNoop;
let Scheduler;
let act;
let use;
+let useState;
let Suspense;
let startTransition;
let pendingTextRequests;
@@ -18,6 +19,7 @@ describe('ReactThenable', () => {
Scheduler = require('scheduler');
act = require('jest-react').act;
use = React.use;
+ useState = React.useState;
Suspense = React.Suspense;
startTransition = React.startTransition;
@@ -668,4 +670,92 @@ describe('ReactThenable', () => {
expect(Scheduler).toHaveYielded(['Hi']);
expect(root).toMatchRenderedOutput('Hi');
});
+
+ // @gate enableUseHook
+ test('does not suspend indefinitely if an interleaved update was skipped', async () => {
+ function Child({childShouldSuspend}) {
+ return (
+
+ );
+ }
+
+ let setChildShouldSuspend;
+ let setShowChild;
+ function Parent() {
+ const [showChild, _setShowChild] = useState(true);
+ setShowChild = _setShowChild;
+
+ const [childShouldSuspend, _setChildShouldSuspend] = useState(false);
+ setChildShouldSuspend = _setChildShouldSuspend;
+
+ Scheduler.unstable_yieldValue(
+ `childShouldSuspend: ${childShouldSuspend}, showChild: ${showChild}`,
+ );
+ return showChild ? (
+
+ ) : (
+
+ );
+ }
+
+ const root = ReactNoop.createRoot();
+ await act(() => {
+ root.render();
+ });
+ expect(Scheduler).toHaveYielded([
+ 'childShouldSuspend: false, showChild: true',
+ 'Child',
+ ]);
+ expect(root).toMatchRenderedOutput('Child');
+
+ await act(() => {
+ // Perform an update that causes the app to suspend
+ startTransition(() => {
+ setChildShouldSuspend(true);
+ });
+ expect(Scheduler).toFlushAndYieldThrough([
+ 'childShouldSuspend: true, showChild: true',
+ ]);
+ // While the update is in progress, schedule another update.
+ startTransition(() => {
+ setShowChild(false);
+ });
+ });
+ expect(Scheduler).toHaveYielded([
+ // Because the interleaved update is not higher priority than what we were
+ // already working on, it won't interrupt. The first update will continue,
+ // and will suspend.
+ 'Async text requested [Will never resolve]',
+
+ // Instead of waiting for the promise to resolve, React notices there's
+ // another pending update that it hasn't tried yet. It will switch to
+ // rendering that instead.
+ //
+ // This time, the update hides the component that previous was suspending,
+ // so it finishes successfully.
+ 'childShouldSuspend: false, showChild: false',
+ '(empty)',
+
+ // Finally, React attempts to render the first update again. It also
+ // finishes successfully, because it was rebased on top of the update that
+ // hid the suspended component.
+ // NOTE: These this render happened to not be entangled with the previous
+ // one. If they had been, this update would have been included in the
+ // previous render, and there wouldn't be an extra one here. This could
+ // change if we change our entanglement heurstics. Semantically, it
+ // shouldn't matter, though in general we try to work on transitions in
+ // parallel whenever possible. So even though in this particular case, the
+ // extra render is unnecessary, it's a nice property that it wasn't
+ // entangled with the other transition.
+ 'childShouldSuspend: true, showChild: false',
+ '(empty)',
+ ]);
+ expect(root).toMatchRenderedOutput('(empty)');
+ });
});