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(