Revert "Throttle retries even if everything has loaded (facebook#26611)"

This reverts commit 8256781.

Author: kassens

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -1085,87 +1085,108 @@ function finishConcurrentRender(
   finishedWork: Fiber,
   lanes: Lanes,
 ) {
-  // TODO: The fact that most of these branches are identical suggests that some
-  // of the exit statuses are not best modeled as exit statuses and should be
-  // tracked orthogonally.
   switch (exitStatus) {
     case RootInProgress:
     case RootFatalErrored: {
       throw new Error('Root did not complete. This is a bug in React.');
     }
+    case RootErrored: {
+      // We should have already attempted to retry this tree. If we reached
+      // this point, it errored again. Commit it.
+      commitRootWhenReady(
+        root,
+        finishedWork,
+        workInProgressRootRecoverableErrors,
+        workInProgressTransitions,
+        lanes,
+      );
+      break;
+    }
+    case RootSuspended: {
+      markRootSuspended(root, lanes);
+
+      // We have an acceptable loading state. We need to figure out if we
+      // should immediately commit it or wait a bit.
+
+      if (
+        includesOnlyRetries(lanes) &&
+        // do not delay if we're inside an act() scope
+        !shouldForceFlushFallbacksInDEV()
+      ) {
+        // This render only included retries, no updates. Throttle committing
+        // retries so that we don't show too many loading states too quickly.
+        const msUntilTimeout =
+          globalMostRecentFallbackTime + FALLBACK_THROTTLE_MS - now();
+        // Don't bother with a very short suspense time.
+        if (msUntilTimeout > 10) {
+          const nextLanes = getNextLanes(root, NoLanes);
+          if (nextLanes !== NoLanes) {
+            // There's additional work on this root.
+            break;
+          }
+
+          // The render is suspended, it hasn't timed out, and there's no
+          // lower priority work to do. Instead of committing the fallback
+          // immediately, wait for more data to arrive.
+          root.timeoutHandle = scheduleTimeout(
+            commitRootWhenReady.bind(
+              null,
+              root,
+              finishedWork,
+              workInProgressRootRecoverableErrors,
+              workInProgressTransitions,
+              lanes,
+            ),
+            msUntilTimeout,
+          );
+          break;
+        }
+      }
+      // The work expired. Commit immediately.
+      commitRootWhenReady(
+        root,
+        finishedWork,
+        workInProgressRootRecoverableErrors,
+        workInProgressTransitions,
+        lanes,
+      );
+      break;
+    }
     case RootSuspendedWithDelay: {
+      markRootSuspended(root, lanes);
+
       if (includesOnlyTransitions(lanes)) {
         // This is a transition, so we should exit without committing a
         // placeholder and without scheduling a timeout. Delay indefinitely
         // until we receive more data.
-        markRootSuspended(root, lanes);
-        return;
+        break;
       }
+
       // Commit the placeholder.
+      commitRootWhenReady(
+        root,
+        finishedWork,
+        workInProgressRootRecoverableErrors,
+        workInProgressTransitions,
+        lanes,
+      );
       break;
     }
-    case RootErrored:
-    case RootSuspended:
     case RootCompleted: {
+      // The work completed.
+      commitRootWhenReady(
+        root,
+        finishedWork,
+        workInProgressRootRecoverableErrors,
+        workInProgressTransitions,
+        lanes,
+      );
       break;
     }
     default: {
       throw new Error('Unknown root exit status.');
     }
   }
-
-  if (shouldForceFlushFallbacksInDEV()) {
-    // We're inside an `act` scope. Commit immediately.
-    commitRoot(
-      root,
-      workInProgressRootRecoverableErrors,
-      workInProgressTransitions,
-    );
-  } else {
-    if (includesOnlyRetries(lanes)) {
-      // This render only included retries, no updates. Throttle committing
-      // retries so that we don't show too many loading states too quickly.
-      const msUntilTimeout =
-        globalMostRecentFallbackTime + FALLBACK_THROTTLE_MS - now();
-
-      // Don't bother with a very short suspense time.
-      if (msUntilTimeout > 10) {
-        markRootSuspended(root, lanes);
-
-        const nextLanes = getNextLanes(root, NoLanes);
-        if (nextLanes !== NoLanes) {
-          // There's additional work we can do on this root. We might as well
-          // attempt to work on that while we're suspended.
-          return;
-        }
-
-        // The render is suspended, it hasn't timed out, and there's no
-        // lower priority work to do. Instead of committing the fallback
-        // immediately, wait for more data to arrive.
-        // TODO: Combine retry throttling with Suspensey commits. Right now they
-        // run one after the other.
-        root.timeoutHandle = scheduleTimeout(
-          commitRootWhenReady.bind(
-            null,
-            root,
-            finishedWork,
-            workInProgressRootRecoverableErrors,
-            workInProgressTransitions,
-            lanes,
-          ),
-          msUntilTimeout,
-        );
-        return;
-      }
-    }
-    commitRootWhenReady(
-      root,
-      finishedWork,
-      workInProgressRootRecoverableErrors,
-      workInProgressTransitions,
-      lanes,
-    );
-  }
 }
 
 function commitRootWhenReady(
@@ -1175,8 +1196,6 @@ function commitRootWhenReady(
   transitions: Array<Transition> | null,
   lanes: Lanes,
 ) {
-  // TODO: Combine retry throttling with Suspensey commits. Right now they run
-  // one after the other.
   if (includesOnlyNonUrgentLanes(lanes)) {
     // Before committing, ask the renderer whether the host tree is ready.
     // If it's not, we'll wait until it notifies us.
@@ -1199,15 +1218,22 @@ function commitRootWhenReady(
       // us that it's ready. This will be canceled if we start work on the
       // root again.
       root.cancelPendingCommit = schedulePendingCommit(
-        commitRoot.bind(null, root, recoverableErrors, transitions),
+        commitRoot.bind(
+          null,
+          root,
+          workInProgressRootRecoverableErrors,
+          workInProgressTransitions,
+        ),
       );
-      markRootSuspended(root, lanes);
       return;
     }
   }
-
   // Otherwise, commit immediately.
-  commitRoot(root, recoverableErrors, transitions);
+  commitRoot(
+    root,
+    workInProgressRootRecoverableErrors,
+    workInProgressTransitions,
+  );
 }
 
 function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean {

packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js

@@ -1016,6 +1016,49 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     expect(ReactNoop).toMatchRenderedOutput(<span prop="C" />);
   });
 
+  // TODO: This test was written against the old Expiration Times
+  // implementation. It doesn't really test what it was intended to test
+  // anymore, because all updates to the same queue get entangled together.
+  // Even if they haven't expired. Consider either deleting or rewriting.
+  // @gate enableLegacyCache
+  it('flushes all expired updates in a single batch', async () => {
+    class Foo extends React.Component {
+      componentDidUpdate() {
+        Scheduler.log('Commit: ' + this.props.text);
+      }
+      componentDidMount() {
+        Scheduler.log('Commit: ' + this.props.text);
+      }
+      render() {
+        return (
+          <Suspense fallback={<Text text="Loading..." />}>
+            <AsyncText text={this.props.text} />
+          </Suspense>
+        );
+      }
+    }
+
+    ReactNoop.render(<Foo text="" />);
+    ReactNoop.expire(1000);
+    jest.advanceTimersByTime(1000);
+    ReactNoop.render(<Foo text="go" />);
+    ReactNoop.expire(1000);
+    jest.advanceTimersByTime(1000);
+    ReactNoop.render(<Foo text="good" />);
+    ReactNoop.expire(1000);
+    jest.advanceTimersByTime(1000);
+    ReactNoop.render(<Foo text="goodbye" />);
+
+    await waitForAll(['Suspend! [goodbye]', 'Loading...', 'Commit: goodbye']);
+    expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
+
+    await resolveText('goodbye');
+    expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
+
+    await waitForAll(['goodbye']);
+    expect(ReactNoop).toMatchRenderedOutput(<span prop="goodbye" />);
+  });
+
   // @gate enableLegacyCache
   it('a suspended update that expires', async () => {
     // Regression test. This test used to fall into an infinite loop.
@@ -1737,7 +1780,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
   });
 
   // @gate enableLegacyCache
-  it('throttles content from appearing if a fallback was shown recently', async () => {
+  it('does suspend if a fallback has been shown for a short time', async () => {
     function Foo() {
       Scheduler.log('Foo');
       return (
@@ -1779,10 +1822,23 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       await resolveText('B');
       expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
 
-      // Restart and render the complete content. The tree will finish but we
-      // won't commit the result yet because the fallback appeared recently.
+      // Restart and render the complete content.
       await waitForAll(['A', 'B']);
-      expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
+      // TODO: Because this render was the result of a retry, and a fallback
+      // was shown recently, we should suspend and remain on the fallback
+      // for little bit longer. We currently only do this if there's still
+      // remaining fallbacks in the tree, but we should do it for all retries.
+      //
+      // Correct output:
+      // expect(ReactNoop).toMatchRenderedOutput(<span prop="Loading..." />);
+      //
+      // Actual output:
+      expect(ReactNoop).toMatchRenderedOutput(
+        <>
+          <span prop="A" />
+          <span prop="B" />
+        </>,
+      );
     });
     assertLog([]);
     expect(ReactNoop).toMatchRenderedOutput(