use: Don't suspend if there are pending updates

acdlite · acdlite · commit 9dfbd9fa90a6 · 2022-11-17T14:48:42.000-05:00
Before suspending, check if there are other pending updates that might
possibly unblock the suspended component. If so, interrupt the current
render and switch to working on that.

This logic was already implemented for the old "throw a Promise"
Suspense but has to be replicated for `use` because it suspends the
work loop much earlier.

I'm getting a little anxious about the divergence between the two
Suspense patterns. I'm going to look into enabling the new behavior for
the old pattern so that we can unify the implementations.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -1849,14 +1849,26 @@ 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)) {
     // We can always wait during a retry.
     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
@@ -1849,14 +1849,26 @@ 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)) {
     // We can always wait during a retry.
     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
@@ -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 (
+        <Text
+          text={
+            childShouldSuspend
+              ? use(getAsyncText('Will never resolve'))
+              : 'Child'
+          }
+        />
+      );
+    }
+
+    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 ? (
+        <Child childShouldSuspend={childShouldSuspend} />
+      ) : (
+        <Text text="(empty)" />
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    await act(() => {
+      root.render(<Parent />);
+    });
+    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)');
+  });
 });
