diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 934fa7327e..6f00533d30 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/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 | 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 { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 2171046831..29948542ec 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -1016,6 +1016,49 @@ 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. @@ -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(); - // 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(); + // 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( + <> + + + , + ); }); assertLog([]); expect(ReactNoop).toMatchRenderedOutput(