From 8256781fdf3ae1947a7f27ddc78ae11b9989c2cd Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 12 Apr 2023 20:25:32 -0400 Subject: [PATCH] Throttle retries even if everything has loaded (#26611) If a Suspense fallback is shown, and the data finishes loading really quickly after that, we throttle the content from appearing for 500ms to reduce thrash. This already works for successive fallback states (like if one fallback is nested inside another) but it wasn't being applied to the final step in the sequence: if there were no more unresolved Suspense boundaries in the tree, the content would appear immediately. This fixes the throttling behavior so that it applies to all renders that are the result of suspended data being loaded. (Our internal jargon term for this is a "retry".) --- .../src/ReactFiberWorkLoop.js | 158 ++++++++---------- .../ReactSuspenseWithNoopRenderer-test.js | 64 +------ 2 files changed, 70 insertions(+), 152 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 6f00533d30..934fa7327e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -1085,108 +1085,87 @@ 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. - break; + markRootSuspended(root, lanes); + return; } - // 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( @@ -1196,6 +1175,8 @@ function commitRootWhenReady( transitions: Array | 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. @@ -1218,22 +1199,15 @@ 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, - workInProgressRootRecoverableErrors, - workInProgressTransitions, - ), + commitRoot.bind(null, root, recoverableErrors, transitions), ); + markRootSuspended(root, lanes); return; } } + // Otherwise, commit immediately. - commitRoot( - root, - workInProgressRootRecoverableErrors, - workInProgressTransitions, - ); + commitRoot(root, recoverableErrors, transitions); } function isRenderConsistentWithExternalStores(finishedWork: Fiber): boolean { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 29948542ec..2171046831 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -1016,49 +1016,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop).toMatchRenderedOutput(); }); - // 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 ( - }> - - - ); - } - } - - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - ReactNoop.expire(1000); - jest.advanceTimersByTime(1000); - ReactNoop.render(); - - await waitForAll(['Suspend! [goodbye]', 'Loading...', 'Commit: goodbye']); - expect(ReactNoop).toMatchRenderedOutput(); - - await resolveText('goodbye'); - expect(ReactNoop).toMatchRenderedOutput(); - - await waitForAll(['goodbye']); - expect(ReactNoop).toMatchRenderedOutput(); - }); - // @gate enableLegacyCache it('a suspended update that expires', async () => { // Regression test. This test used to fall into an infinite loop. @@ -1780,7 +1737,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // @gate enableLegacyCache - it('does suspend if a fallback has been shown for a short time', async () => { + it('throttles content from appearing if a fallback was shown recently', async () => { function Foo() { Scheduler.log('Foo'); return ( @@ -1822,23 +1779,10 @@ describe('ReactSuspenseWithNoopRenderer', () => { await resolveText('B'); expect(ReactNoop).toMatchRenderedOutput(); - // Restart and render the complete content. + // Restart and render the complete content. The tree will finish but we + // won't commit the result yet because the fallback appeared recently. await waitForAll(['A', 'B']); - // 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(); - // - // Actual output: - expect(ReactNoop).toMatchRenderedOutput( - <> - - - , - ); + expect(ReactNoop).toMatchRenderedOutput(); }); assertLog([]); expect(ReactNoop).toMatchRenderedOutput(