diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 33472084173c0..6e0d59f001343 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1413,6 +1413,9 @@ function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) { } } +// TODO: This is now an empty object. Should we just make it a boolean? +const SUSPENDED_MARKER: SuspenseState = ({}: any); + function updateSuspenseComponent( current, workInProgress, @@ -1440,17 +1443,9 @@ function updateSuspenseComponent( (ForceSuspenseFallback: SuspenseContext), ) ) { - // This either already captured or is a new mount that was forced into its fallback - // state by a parent. - const attemptedState: SuspenseState | null = workInProgress.memoizedState; // Something in this boundary's subtree already suspended. Switch to // rendering the fallback children. - nextState = { - fallbackExpirationTime: - attemptedState !== null - ? attemptedState.fallbackExpirationTime - : NoWork, - }; + nextState = SUSPENDED_MARKER; nextDidTimeout = true; workInProgress.effectTag &= ~DidCapture; } else { diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 83f3fe1b72dd4..63f43527de167 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -63,10 +63,6 @@ import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; import warning from 'shared/warning'; -import { - NoWork, - computeAsyncExpirationNoBucket, -} from './ReactFiberExpirationTime'; import {onCommitUnmount} from './ReactFiberDevToolsHook'; import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf'; import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; @@ -102,8 +98,8 @@ import { } from './ReactFiberHostConfig'; import { captureCommitPhaseError, - requestCurrentTime, resolveRetryThenable, + markCommitTimeOfFallback, } from './ReactFiberWorkLoop'; import { NoEffect as NoHookEffect, @@ -1288,16 +1284,7 @@ function commitSuspenseComponent(finishedWork: Fiber) { } else { newDidTimeout = true; primaryChildParent = finishedWork.child; - if (newState.fallbackExpirationTime === NoWork) { - // If the children had not already timed out, record the time. - // This is used to compute the elapsed time during subsequent - // attempts to render the children. - // We model this as a normal pri expiration time since that's - // how we infer start time for updates. - newState.fallbackExpirationTime = computeAsyncExpirationNoBucket( - requestCurrentTime(), - ); - } + markCommitTimeOfFallback(); } if (supportsMutation && primaryChildParent !== null) { diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index a75b359a484a9..0b8ad6e63aac4 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -101,7 +101,6 @@ import { enableEventAPI, } from 'shared/ReactFeatureFlags'; import { - markRenderEventTimeAndConfig, renderDidSuspend, renderDidSuspendDelayIfPossible, } from './ReactFiberWorkLoop'; @@ -702,14 +701,6 @@ function completeWork( prevDidTimeout = prevState !== null; if (!nextDidTimeout && prevState !== null) { // We just switched from the fallback to the normal children. - - // Mark the event time of the switching from fallback to normal children, - // based on the start of when we first showed the fallback. This time - // was given a normal pri expiration time at the time it was shown. - const fallbackExpirationTime: ExpirationTime = - prevState.fallbackExpirationTime; - markRenderEventTimeAndConfig(fallbackExpirationTime, null); - // Delete the fallback. // TODO: Would it be better to store the fallback fragment on // the stateNode during the begin phase? @@ -737,6 +728,13 @@ function completeWork( // in the concurrent tree already suspended during this render. // This is a known bug. if ((workInProgress.mode & BatchedMode) !== NoMode) { + // TODO: Move this back to throwException because this is too late + // if this is a large tree which is common for initial loads. We + // don't know if we should restart a render or not until we get + // this marker, and this is too late. + // If this render already had a ping or lower pri updates, + // and this is the first time we know we're going to suspend we + // should be able to immediately restart from within throwException. const hasInvisibleChildContext = current === null && workInProgress.memoizedProps.unstable_avoidThisFallback !== true; diff --git a/packages/react-reconciler/src/ReactFiberExpirationTime.js b/packages/react-reconciler/src/ReactFiberExpirationTime.js index d3e2e20161010..652ca93aed3be 100644 --- a/packages/react-reconciler/src/ReactFiberExpirationTime.js +++ b/packages/react-reconciler/src/ReactFiberExpirationTime.js @@ -83,14 +83,6 @@ export function computeSuspenseExpiration( ); } -// Same as computeAsyncExpiration but without the bucketing logic. This is -// used to compute timestamps instead of actual expiration times. -export function computeAsyncExpirationNoBucket( - currentTime: ExpirationTime, -): ExpirationTime { - return currentTime - LOW_PRIORITY_EXPIRATION / UNIT_SIZE; -} - // We intentionally set a higher expiration time for interactive updates in // dev than in production. // diff --git a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js index 40db21ebdba0d..ba80272c054b2 100644 --- a/packages/react-reconciler/src/ReactFiberSuspenseComponent.js +++ b/packages/react-reconciler/src/ReactFiberSuspenseComponent.js @@ -8,11 +8,9 @@ */ import type {Fiber} from './ReactFiber'; -import type {ExpirationTime} from './ReactFiberExpirationTime'; -export type SuspenseState = {| - fallbackExpirationTime: ExpirationTime, -|}; +// TODO: This is now an empty object. Should we switch this to a boolean? +export type SuspenseState = {||}; export function shouldCaptureSuspense( workInProgress: Fiber, diff --git a/packages/react-reconciler/src/ReactFiberThrow.js b/packages/react-reconciler/src/ReactFiberThrow.js index d6c57d44f833e..5933ebf21925c 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.js +++ b/packages/react-reconciler/src/ReactFiberThrow.js @@ -275,6 +275,45 @@ function throwException( // Confirmed that the boundary is in a concurrent mode tree. Continue // with the normal suspend path. + // + // After this we'll use a set of heuristics to determine whether this + // render pass will run to completion or restart or "suspend" the commit. + // The actual logic for this is spread out in different places. + // + // This first principle is that if we're going to suspend when we complete + // a root, then we should also restart if we get an update or ping that + // might unsuspend it, and vice versa. The only reason to suspend is + // because you think you might want to restart before committing. However, + // it doesn't make sense to restart only while in the period we're suspended. + // + // Restarting too aggressively is also not good because it starves out any + // intermediate loading state. So we use heuristics to determine when. + + // Suspense Heuristics + // + // If nothing threw a Promise or all the same fallbacks are already showing, + // then don't suspend/restart. + // + // If this is an initial render of a new tree of Suspense boundaries and + // those trigger a fallback, then don't suspend/restart. We want to ensure + // that we can show the initial loading state as quickly as possible. + // + // If we hit a "Delayed" case, such as when we'd switch from content back into + // a fallback, then we should always suspend/restart. SuspenseConfig applies to + // this case. If none is defined, JND is used instead. + // + // If we're already showing a fallback and it gets "retried", allowing us to show + // another level, but there's still an inner boundary that would show a fallback, + // then we suspend/restart for 500ms since the last time we showed a fallback + // anywhere in the tree. This effectively throttles progressive loading into a + // consistent train of commits. This also gives us an opportunity to restart to + // get to the completed state slightly earlier. + // + // If there's ambiguity due to batching it's resolved in preference of: + // 1) "delayed", 2) "initial render", 3) "retry". + // + // We want to ensure that a "busy" state doesn't get force committed. We want to + // ensure that new initial loading states can commit as soon as possible. attachPingListener(root, renderExpirationTime, thenable); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 270299d5f731f..30b380de3fb5a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -213,6 +213,14 @@ let workInProgressRootExitStatus: RootExitStatus = RootIncomplete; let workInProgressRootLatestProcessedExpirationTime: ExpirationTime = Sync; let workInProgressRootLatestSuspenseTimeout: ExpirationTime = Sync; let workInProgressRootCanSuspendUsingConfig: null | SuspenseConfig = null; +// If we're pinged while rendering we don't always restart immediately. +// This flag determines if it might be worthwhile to restart if an opportunity +// happens latere. +let workInProgressRootHasPendingPing: boolean = false; +// The most recent time we committed a fallback. This lets us ensure a train +// model where we don't commit new loading states in too quick succession. +let globalMostRecentFallbackTime: number = 0; +const FALLBACK_THROTTLE_MS: number = 500; let nextEffect: Fiber | null = null; let hasUncaughtError = false; @@ -753,6 +761,7 @@ function prepareFreshStack(root, expirationTime) { workInProgressRootLatestProcessedExpirationTime = Sync; workInProgressRootLatestSuspenseTimeout = Sync; workInProgressRootCanSuspendUsingConfig = null; + workInProgressRootHasPendingPing = false; if (__DEV__) { ReactStrictModeWarnings.discardPendingWarnings(); @@ -794,6 +803,26 @@ function renderRoot( if (root !== workInProgressRoot || expirationTime !== renderExpirationTime) { prepareFreshStack(root, expirationTime); startWorkOnPendingInteraction(root, expirationTime); + } else if (workInProgressRootExitStatus === RootSuspendedWithDelay) { + // We could've received an update at a lower priority while we yielded. + // We're suspended in a delayed state. Once we complete this render we're + // just going to try to recover at the last pending time anyway so we might + // as well start doing that eagerly. + // Ideally we should be able to do this even for retries but we don't yet + // know if we're going to process an update which wants to commit earlier, + // and this path happens very early so it would happen too often. Instead, + // for that case, we'll wait until we complete. + if (workInProgressRootHasPendingPing) { + // We have a ping at this expiration. Let's restart to see if we get unblocked. + prepareFreshStack(root, expirationTime); + } else { + const lastPendingTime = root.lastPendingTime; + if (lastPendingTime < expirationTime) { + // There's lower priority work. It might be unsuspended. Try rendering + // at that level immediately, while preserving the position in the queue. + return renderRoot.bind(null, root, lastPendingTime); + } + } } // If we have a work-in-progress fiber, it means there's still work to do @@ -932,7 +961,7 @@ function renderRoot( // An error was thrown. First check if there is lower priority work // scheduled on this root. const lastPendingTime = root.lastPendingTime; - if (root.lastPendingTime < expirationTime) { + if (lastPendingTime < expirationTime) { // There's lower priority work. Before raising the error, try rendering // at the lower priority to see if it fixes it. Use a continuation to // maintain the existing priority and position in the queue. @@ -950,39 +979,114 @@ function renderRoot( // errored state. return commitRoot.bind(null, root); } - case RootSuspended: + case RootSuspended: { + // We have an acceptable loading state. We need to figure out if we should + // immediately commit it or wait a bit. + + // If we have processed new updates during this render, we may now have a + // new loading state ready. We want to ensure that we commit that as soon as + // possible. + const hasNotProcessedNewUpdates = + workInProgressRootLatestProcessedExpirationTime === Sync; + if (hasNotProcessedNewUpdates && !disableYielding && !isSync) { + // If we have not processed any new updates during this pass, then this is + // either a retry of an existing fallback state or a hidden tree. + // Hidden trees shouldn't be batched with other work and after that's + // fixed it can only be a retry. + // We're going to throttle committing retries so that we don't show too + // many loading states too quickly. + let msUntilTimeout = + globalMostRecentFallbackTime + FALLBACK_THROTTLE_MS - now(); + // Don't bother with a very short suspense time. + if (msUntilTimeout > 10) { + if (workInProgressRootHasPendingPing) { + // This render was pinged but we didn't get to restart earlier so try + // restarting now instead. + prepareFreshStack(root, expirationTime); + return renderRoot.bind(null, root, expirationTime); + } + const lastPendingTime = root.lastPendingTime; + if (lastPendingTime < expirationTime) { + // There's lower priority work. It might be unsuspended. Try rendering + // at that level. + return renderRoot.bind(null, root, lastPendingTime); + } + // 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( + commitRoot.bind(null, root), + msUntilTimeout, + ); + return null; + } + } + // The work expired. Commit immediately. + return commitRoot.bind(null, root); + } case RootSuspendedWithDelay: { - if (!isSync) { + if (!disableYielding && !isSync) { + // We're suspended in a state that should be avoided. We'll try to avoid committing + // it for as long as the timeouts let us. + if (workInProgressRootHasPendingPing) { + // This render was pinged but we didn't get to restart earlier so try + // restarting now instead. + prepareFreshStack(root, expirationTime); + return renderRoot.bind(null, root, expirationTime); + } const lastPendingTime = root.lastPendingTime; - if (root.lastPendingTime < expirationTime) { + if (lastPendingTime < expirationTime) { // There's lower priority work. It might be unsuspended. Try rendering - // at that level. + // at that level immediately. return renderRoot.bind(null, root, lastPendingTime); } - // If workInProgressRootLatestProcessedExpirationTime is Sync, that means we didn't - // track any event times. That can happen if we retried but nothing switched - // from fallback to content. There's no reason to delay doing no work. - if (workInProgressRootLatestProcessedExpirationTime !== Sync) { - let shouldDelay = - workInProgressRootExitStatus === RootSuspendedWithDelay; - let msUntilTimeout = computeMsUntilTimeout( + + let msUntilTimeout; + if (workInProgressRootLatestSuspenseTimeout !== Sync) { + // We have processed a suspense config whose expiration time we can use as + // the timeout. + msUntilTimeout = + expirationTimeToMs(workInProgressRootLatestSuspenseTimeout) - now(); + } else if (workInProgressRootLatestProcessedExpirationTime === Sync) { + // This should never normally happen because only new updates cause + // delayed states, so we should have processed something. However, + // this could also happen in an offscreen tree. + msUntilTimeout = 0; + } else { + // If we don't have a suspense config, we're going to use a heuristic to + // determine how long we can suspend. + const eventTimeMs: number = inferTimeFromExpirationTime( workInProgressRootLatestProcessedExpirationTime, - workInProgressRootLatestSuspenseTimeout, - expirationTime, - workInProgressRootCanSuspendUsingConfig, - shouldDelay, ); - // Don't bother with a very short suspense time. - if (msUntilTimeout > 10) { - // 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( - commitRoot.bind(null, root), - msUntilTimeout, - ); - return null; + const currentTimeMs = now(); + const timeUntilExpirationMs = + expirationTimeToMs(expirationTime) - currentTimeMs; + let timeElapsed = currentTimeMs - eventTimeMs; + if (timeElapsed < 0) { + // We get this wrong some time since we estimate the time. + timeElapsed = 0; } + + msUntilTimeout = jnd(timeElapsed) - timeElapsed; + + // Clamp the timeout to the expiration time. + // TODO: Once the event time is exact instead of inferred from expiration time + // we don't need this. + if (timeUntilExpirationMs < msUntilTimeout) { + msUntilTimeout = timeUntilExpirationMs; + } + } + + // Don't bother with a very short suspense time. + if (msUntilTimeout > 10) { + // 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( + commitRoot.bind(null, root), + msUntilTimeout, + ); + return null; } } // The work expired. Commit immediately. @@ -1019,6 +1123,10 @@ function renderRoot( } } +export function markCommitTimeOfFallback() { + globalMostRecentFallbackTime = now(); +} + export function markRenderEventTimeAndConfig( expirationTime: ExpirationTime, suspenseConfig: null | SuspenseConfig, @@ -1062,18 +1170,24 @@ export function renderDidError() { } } -function inferTimeFromExpirationTime( +function inferTimeFromExpirationTime(expirationTime: ExpirationTime): number { + // We don't know exactly when the update was scheduled, but we can infer an + // approximate start time from the expiration time. + const earliestExpirationTimeMs = expirationTimeToMs(expirationTime); + return earliestExpirationTimeMs - LOW_PRIORITY_EXPIRATION; +} + +function inferTimeFromExpirationTimeWithSuspenseConfig( expirationTime: ExpirationTime, - suspenseConfig: null | SuspenseConfig, + suspenseConfig: SuspenseConfig, ): number { // We don't know exactly when the update was scheduled, but we can infer an - // approximate start time from the expiration time. + // approximate start time from the expiration time by subtracting the timeout + // that was added to the event time. const earliestExpirationTimeMs = expirationTimeToMs(expirationTime); return ( earliestExpirationTimeMs - - (suspenseConfig !== null - ? suspenseConfig.timeoutMs | 0 || LOW_PRIORITY_EXPIRATION - : LOW_PRIORITY_EXPIRATION) + (suspenseConfig.timeoutMs | 0 || LOW_PRIORITY_EXPIRATION) ); } @@ -1879,9 +1993,32 @@ export function pingSuspendedRoot( if (workInProgressRoot === root && renderExpirationTime === suspendedTime) { // Received a ping at the same priority level at which we're currently - // rendering. Restart from the root. Don't need to schedule a ping because - // we're already working on this tree. - prepareFreshStack(root, renderExpirationTime); + // rendering. We might want to restart this render. This should mirror + // the logic of whether or not a root suspends once it completes. + + // TODO: If we're rendering sync either due to Sync, Batched or expired, + // we should probably never restart. + + // If we're suspended with delay, we'll always suspend so we can always + // restart. If we're suspended without any updates, it might be a retry. + // If it's early in the retry we can restart. We can't know for sure + // whether we'll eventually process an update during this render pass, + // but it's somewhat unlikely that we get to a ping before that, since + // getting to the root most update is usually very fast. + if ( + workInProgressRootExitStatus === RootSuspendedWithDelay || + (workInProgressRootExitStatus === RootSuspended && + workInProgressRootLatestProcessedExpirationTime === Sync && + now() - globalMostRecentFallbackTime < FALLBACK_THROTTLE_MS) + ) { + // Restart from the root. Don't need to schedule a ping because + // we're already working on this tree. + prepareFreshStack(root, renderExpirationTime); + } else { + // Even though we can't restart right now, we might get an + // opportunity later. So we mark this render as having a ping. + workInProgressRootHasPendingPing = true; + } return; } @@ -2007,7 +2144,7 @@ function computeMsUntilSuspenseLoadingDelay( // Compute the time until this render pass would expire. const currentTimeMs: number = now(); - const eventTimeMs: number = inferTimeFromExpirationTime( + const eventTimeMs: number = inferTimeFromExpirationTimeWithSuspenseConfig( mostRecentEventTime, suspenseConfig, ); @@ -2022,52 +2159,6 @@ function computeMsUntilSuspenseLoadingDelay( return msUntilTimeout; } -function computeMsUntilTimeout( - mostRecentEventTime: ExpirationTime, - suspenseTimeout: ExpirationTime, - committedExpirationTime: ExpirationTime, - suspenseConfig: null | SuspenseConfig, - shouldDelay: boolean, -) { - if (disableYielding) { - // Timeout immediately when yielding is disabled. - return 0; - } - - // Compute the time until this render pass would expire. - const currentTimeMs: number = now(); - - if (suspenseTimeout !== Sync && shouldDelay) { - const timeUntilTimeoutMs = - expirationTimeToMs(suspenseTimeout) - currentTimeMs; - return timeUntilTimeoutMs; - } - - const eventTimeMs: number = inferTimeFromExpirationTime( - mostRecentEventTime, - suspenseConfig, - ); - const timeUntilExpirationMs = - expirationTimeToMs(committedExpirationTime) - currentTimeMs; - let timeElapsed = currentTimeMs - eventTimeMs; - if (timeElapsed < 0) { - // We get this wrong some time since we estimate the time. - timeElapsed = 0; - } - - let msUntilTimeout = jnd(timeElapsed) - timeElapsed; - - // Clamp the timeout to the expiration time. - // TODO: Once the event time is exact instead of inferred from expiration time - // we don't need this. - if (timeUntilExpirationMs < msUntilTimeout) { - msUntilTimeout = timeUntilExpirationMs; - } - - // This is the value that is passed to `setTimeout`. - return msUntilTimeout; -} - function checkForNestedUpdates() { if (nestedUpdateCount > NESTED_UPDATE_LIMIT) { nestedUpdateCount = 0; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js index 282ab0a0787fd..15212799aa5e4 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalPerf-test.internal.js @@ -595,6 +595,16 @@ describe('ReactDebugFiberPerf', () => { }), ); + // Initial render + ReactNoop.render( + + } /> + , + ); + expect(Scheduler).toFlushWithoutYielding(); + expect(getFlameChart()).toMatchSnapshot(); + + // Update that suspends ReactNoop.render( }> diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index 195f98fd7a53a..4b241e08c52ab 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -46,7 +46,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('Hi'); await Promise.resolve(); @@ -136,13 +136,13 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('FooBar'); jest.advanceTimersByTime(100); await promiseForFoo; - expect(Scheduler).toFlushAndYield(['Foo', 'Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(Scheduler).toFlushAndYield(['Foo']); + expect(root).not.toMatchRenderedOutput('FooBar'); jest.advanceTimersByTime(500); await promiseForBar; @@ -165,7 +165,7 @@ describe('ReactLazy', () => { }, ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('Hi'); await Promise.resolve(); @@ -193,7 +193,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('Hi'); try { await Promise.resolve(); @@ -239,7 +239,7 @@ describe('ReactLazy', () => { }); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('AB'); await LazyChildA; await LazyChildB; @@ -280,7 +280,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('Hi'); await Promise.resolve(); @@ -330,7 +330,7 @@ describe('ReactLazy', () => { }, ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('SiblingA'); await Promise.resolve(); @@ -403,7 +403,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('A1'); await Promise.resolve(); @@ -536,7 +536,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('Hi Bye'); await Promise.resolve(); expect(Scheduler).toFlushAndYield(['Hi Bye']); @@ -572,7 +572,6 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); await Promise.resolve(); root.update( @@ -600,7 +599,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('Hello'); await Promise.resolve(); root.update( @@ -650,7 +649,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('22'); // Mount await Promise.resolve(); @@ -836,7 +835,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('Inner default text'); // Mount await Promise.resolve(); @@ -878,7 +877,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Started loading', 'Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput(
AB
); await Promise.resolve(); @@ -924,7 +923,7 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('FooBar'); expect(ref.current).toBe(null); await Promise.resolve(); @@ -952,7 +951,7 @@ describe('ReactLazy', () => { }, ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('4'); // Mount await Promise.resolve(); @@ -1036,7 +1035,7 @@ describe('ReactLazy', () => { }, ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); + expect(root).not.toMatchRenderedOutput('4'); // Mount await Promise.resolve(); @@ -1069,7 +1068,7 @@ describe('ReactLazy', () => { }); const ref = React.createRef(); - const root = ReactTestRenderer.create( + ReactTestRenderer.create( }> , @@ -1079,7 +1078,6 @@ describe('ReactLazy', () => { ); expect(Scheduler).toFlushAndYield(['Loading...']); - expect(root).toMatchRenderedOutput(null); await Promise.resolve(); expect(() => { expect(Scheduler).toFlushAndYield([]); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index d7a3b894290dc..54ea1f081667c 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -98,22 +98,32 @@ describe('ReactSuspense', () => { return props.children; } - function Foo() { + function Foo({renderBar}) { Scheduler.yieldValue('Foo'); return ( }> - - - - + {renderBar ? ( + + + + + ) : null} ); } + // Render an empty shell const root = ReactTestRenderer.create(, { unstable_isConcurrent: true, }); + expect(Scheduler).toFlushAndYield(['Foo']); + expect(root).toMatchRenderedOutput(null); + + // Navigate the shell to now render the child content. + // This should suspend. + root.update(); + expect(Scheduler).toFlushAndYield([ 'Foo', 'Bar', @@ -161,17 +171,12 @@ describe('ReactSuspense', () => { 'Suspend! [B]', 'Loading B...', ]); - expect(root).toMatchRenderedOutput(null); - - // Advance time by enough to timeout both components and commit their placeholders - jest.advanceTimersByTime(4000); - expect(Scheduler).toFlushWithoutYielding(); expect(root).toMatchRenderedOutput('Loading A...Loading B...'); // Advance time by enough that the first Suspense's promise resolves and // switches back to the normal view. The second Suspense should still // show the placeholder - jest.advanceTimersByTime(1000); + jest.advanceTimersByTime(5000); // TODO: Should we throw if you forget to call toHaveYielded? expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A']); @@ -214,28 +219,48 @@ describe('ReactSuspense', () => { } const root = ReactTestRenderer.create( - }> - - - , + + } /> + + , { unstable_isConcurrent: true, }, ); + expect(Scheduler).toFlushAndYield(['Initial']); + expect(root).toMatchRenderedOutput('Initial'); - expect(Scheduler).toFlushAndYieldThrough(['Suspend!']); + // The update will suspend. + root.update( + + }> + + + + + , + ); + + // Yield past the Suspense boundary but don't complete the last sibling. + expect(Scheduler).toFlushAndYieldThrough([ + 'Suspend!', + 'Loading...', + 'After Suspense', + ]); // The promise resolves before the current render phase has completed resolveThenable(); expect(Scheduler).toHaveYielded([]); + expect(root).toMatchRenderedOutput('Initial'); // Start over from the root, instead of continuing. expect(Scheduler).toFlushAndYield([ // Async renders again *before* Sibling 'Async', + 'After Suspense', 'Sibling', ]); - expect(root).toMatchRenderedOutput('AsyncSibling'); + expect(root).toMatchRenderedOutput('AsyncAfter SuspenseSibling'); }); it('mounts a lazy class component in non-concurrent mode', async () => { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index 9f02055477ad8..878e3cf01bf5e 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -126,7 +126,7 @@ describe('ReactSuspensePlaceholder', () => { ReactNoop.render(); expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'C', 'Loading...']); - expect(ReactNoop).toMatchRenderedOutput(null); + expect(ReactNoop).toMatchRenderedOutput('Loading...'); jest.advanceTimersByTime(1000); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); @@ -189,7 +189,7 @@ describe('ReactSuspensePlaceholder', () => { expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'C', 'Loading...']); - expect(ReactNoop).toMatchRenderedOutput(null); + expect(ReactNoop).not.toMatchRenderedOutput('ABC'); jest.advanceTimersByTime(1000); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); @@ -239,7 +239,7 @@ describe('ReactSuspensePlaceholder', () => { expect(Scheduler).toFlushAndYield(['a', 'Suspend! [b]', 'c', 'Loading...']); - expect(ReactNoop).toMatchRenderedOutput(null); + expect(ReactNoop).toMatchRenderedOutput(LOADING...); jest.advanceTimersByTime(1000); expect(Scheduler).toHaveYielded(['Promise resolved [b]']); @@ -345,10 +345,8 @@ describe('ReactSuspensePlaceholder', () => { 'Text', 'Fallback', ]); - expect(ReactNoop).toMatchRenderedOutput(null); - - // Show the fallback UI. - jest.advanceTimersByTime(750); + // Since this is initial render we immediately commit the fallback. Another test below + // deals with the update case where this suspends. expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(1); @@ -359,7 +357,7 @@ describe('ReactSuspensePlaceholder', () => { expect(onRender.mock.calls[0][3]).toBe(10); // Resolve the pending promise. - jest.advanceTimersByTime(250); + jest.advanceTimersByTime(1000); expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']); expect(Scheduler).toFlushAndYield(['Suspending', 'Loaded', 'Text']); expect(ReactNoop).toMatchRenderedOutput('LoadedText'); @@ -437,7 +435,12 @@ describe('ReactSuspensePlaceholder', () => { }); it('properly accounts for base durations when a suspended times out in a concurrent tree', () => { - ReactNoop.render(); + ReactNoop.render( + + + + , + ); expect(Scheduler).toFlushAndYield(['App', 'Text']); expect(ReactNoop).toMatchRenderedOutput('Text'); @@ -448,7 +451,12 @@ describe('ReactSuspensePlaceholder', () => { expect(onRender.mock.calls[0][2]).toBe(5); expect(onRender.mock.calls[0][3]).toBe(5); - ReactNoop.render(); + ReactNoop.render( + + + + , + ); expect(Scheduler).toFlushAndYield([ 'App', 'Suspending', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 935ec1449aff5..a42136a80b90f 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -108,25 +108,96 @@ describe('ReactSuspenseWithNoopRenderer', () => { ]); }); - it('suspends rendering and continues later', async () => { + it('does not restart rendering for initial render', async () => { function Bar(props) { Scheduler.yieldValue('Bar'); return props.children; } function Foo() { + Scheduler.yieldValue('Foo'); + return ( + + }> + + + + + + + + + ); + } + + ReactNoop.render(); + expect(Scheduler).toFlushAndYieldThrough([ + 'Foo', + 'Bar', + // A suspends + 'Suspend! [A]', + // But we keep rendering the siblings + 'B', + 'Loading...', + 'C', + // We leave D incomplete. + ]); + expect(ReactNoop.getChildren()).toEqual([]); + + // Flush the promise completely + Scheduler.advanceTime(100); + await advanceTimers(100); + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + + // Even though the promise has resolved, we should now flush + // and commit the in progress render instead of restarting. + expect(Scheduler).toFlushAndYield(['D']); + expect(ReactNoop.getChildren()).toEqual([ + span('Loading...'), + span('C'), + span('D'), + ]); + + // Await one micro task to attach the retry listeners. + await null; + + // Next, we'll flush the complete content. + expect(Scheduler).toFlushAndYield(['Bar', 'A', 'B']); + + expect(ReactNoop.getChildren()).toEqual([ + span('A'), + span('B'), + span('C'), + span('D'), + ]); + }); + + it('suspends rendering and continues later', async () => { + function Bar(props) { + Scheduler.yieldValue('Bar'); + return props.children; + } + + function Foo({renderBar}) { Scheduler.yieldValue('Foo'); return ( }> - - - - + {renderBar ? ( + + + + + ) : null} ); } + // Render empty shell. ReactNoop.render(); + expect(Scheduler).toFlushAndYield(['Foo']); + + // The update will suspend. + ReactNoop.render(); expect(Scheduler).toFlushAndYield([ 'Foo', 'Bar', @@ -170,13 +241,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Suspend! [B]', 'Loading B...', ]); - expect(ReactNoop.getChildren()).toEqual([]); - - // Advance time by enough to timeout both components and commit their placeholders - ReactNoop.expire(4000); - await advanceTimers(4000); - - expect(Scheduler).toFlushWithoutYielding(); expect(ReactNoop.getChildren()).toEqual([ span('Loading A...'), span('Loading B...'), @@ -185,8 +249,8 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Advance time by enough that the first Suspense's promise resolves and // switches back to the normal view. The second Suspense should still // show the placeholder - ReactNoop.expire(1000); - await advanceTimers(1000); + ReactNoop.expire(5000); + await advanceTimers(5000); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A']); @@ -203,6 +267,10 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('continues rendering siblings after suspending', async () => { + // A shell is needed. The update cause it to suspend. + ReactNoop.render(} />); + expect(Scheduler).toFlushAndYield([]); + // B suspends. Continue rendering the remaining siblings. ReactNoop.render( }> @@ -253,17 +321,23 @@ describe('ReactSuspenseWithNoopRenderer', () => { } const errorBoundary = React.createRef(); - function App() { + function App({renderContent}) { return ( }> - - - + {renderContent ? ( + + + + ) : null} ); } ReactNoop.render(); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop.getChildren()).toEqual([]); + + ReactNoop.render(); expect(Scheduler).toFlushAndYield(['Suspend! [Result]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); @@ -318,16 +392,11 @@ describe('ReactSuspenseWithNoopRenderer', () => { ReactNoop.render(); expect(Scheduler).toFlushAndYield(['Suspend! [Result]', 'Loading...']); - expect(ReactNoop.getChildren()).toEqual([]); - - ReactNoop.expire(2000); - await advanceTimers(2000); - expect(Scheduler).toFlushWithoutYielding(); expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); textResourceShouldFail = true; - ReactNoop.expire(1000); - await advanceTimers(1000); + ReactNoop.expire(3000); + await advanceTimers(3000); textResourceShouldFail = false; expect(Scheduler).toHaveYielded(['Promise rejected [Result]']); @@ -394,20 +463,24 @@ describe('ReactSuspenseWithNoopRenderer', () => { function App(props) { return ( }> - + {props.showA && } {props.showB && } ); } - ReactNoop.render(); + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop.getChildren()).toEqual([]); + + ReactNoop.render(); expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); // Advance React's virtual time by enough to fall into a new async bucket, // but not enough to expire the suspense timeout. ReactNoop.expire(120); - ReactNoop.render(); + ReactNoop.render(); expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'B', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([]); @@ -448,6 +521,13 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('forces an expiration after an update times out', async () => { + ReactNoop.render( + + } /> + , + ); + expect(Scheduler).toFlushAndYield([]); + ReactNoop.render( }> @@ -485,7 +565,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Async'), span('Sync')]); }); - it('switches to an inner fallback even if it expires later', async () => { + it('switches to an inner fallback after suspending for a while', async () => { // Advance the virtual time so that we're closer to the edge of a bucket. ReactNoop.expire(200); @@ -509,26 +589,15 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Loading inner...', 'Loading outer...', ]); - // The update hasn't expired yet, so we commit nothing. - expect(ReactNoop.getChildren()).toEqual([]); - - // Expire the outer timeout, but don't expire the inner one. - // We should see the outer loading placeholder. - ReactNoop.expire(250); - await advanceTimers(250); - expect(Scheduler).toFlushWithoutYielding(); + // The outer loading state finishes immediately. expect(ReactNoop.getChildren()).toEqual([ span('Sync'), span('Loading outer...'), ]); // Resolve the outer promise. - ReactNoop.expire(50); - await advanceTimers(50); - // At this point, 250ms have elapsed total. The outer placeholder - // timed out at around 150-200ms. So, 50-100ms have elapsed since the - // placeholder timed out. That means we still haven't reached the 150ms - // threshold of the inner placeholder. + ReactNoop.expire(300); + await advanceTimers(300); expect(Scheduler).toHaveYielded(['Promise resolved [Outer content]']); expect(Scheduler).toFlushAndYield([ 'Outer content', @@ -618,6 +687,13 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('expires early by default', async () => { + ReactNoop.render( + + } /> + , + ); + expect(Scheduler).toFlushAndYield([]); + ReactNoop.render( }> @@ -654,17 +730,33 @@ describe('ReactSuspenseWithNoopRenderer', () => { it('resolves successfully even if fallback render is pending', async () => { ReactNoop.render( - }> - - , + + } /> + , + ); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop.getChildren()).toEqual([]); + ReactNoop.render( + + }> + + + , ); expect(ReactNoop.flushNextYield()).toEqual(['Suspend! [Async]']); await advanceTimers(1500); expect(Scheduler).toHaveYielded([]); + expect(ReactNoop.getChildren()).toEqual([]); // Before we have a chance to flush, the promise resolves. await advanceTimers(2000); expect(Scheduler).toHaveYielded(['Promise resolved [Async]']); - expect(Scheduler).toFlushAndYield(['Async']); + expect(Scheduler).toFlushAndYield([ + // We've now pinged the boundary but we don't know if we should restart yet, + // because we haven't completed the suspense boundary. + 'Loading...', + // Once we've completed the boundary we restarted. + 'Async', + ]); expect(ReactNoop.getChildren()).toEqual([span('Async')]); }); @@ -702,6 +794,9 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('can resume rendering earlier than a timeout', async () => { + ReactNoop.render(} />); + expect(Scheduler).toFlushAndYield([]); + ReactNoop.render( }> @@ -1314,16 +1409,19 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('suspends for longer if something took a long (CPU bound) time to render', async () => { - function Foo() { + function Foo({renderContent}) { Scheduler.yieldValue('Foo'); return ( }> - + {renderContent ? : null} ); } ReactNoop.render(); + expect(Scheduler).toFlushAndYield(['Foo']); + + ReactNoop.render(); Scheduler.advanceTime(100); await advanceTimers(100); // Start rendering @@ -1363,7 +1461,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('A')]); }); - it('suspends for longer if a fallback has been shown for a long time', async () => { + it('does not suspends if a fallback has been shown for a long time', async () => { function Foo() { Scheduler.yieldValue('Foo'); return ( @@ -1387,13 +1485,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Loading more...', 'Loading...', ]); - // We're now suspended and we haven't shown anything yet. - expect(ReactNoop.getChildren()).toEqual([]); - - // Show the fallback. - Scheduler.advanceTime(400); - await advanceTimers(400); - expect(Scheduler).toFlushWithoutYielding(); expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); // Wait a long time. @@ -1408,42 +1499,92 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'Suspend! [B]', 'Loading more...', ]); - // Because we've already been waiting for so long we can - // wait a bit longer. Still nothing... - Scheduler.advanceTime(600); - await advanceTimers(600); - expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); - - // Eventually we'll show more content with inner fallback. - Scheduler.advanceTime(3000); - await advanceTimers(3000); - // No need to rerender. - expect(Scheduler).toFlushWithoutYielding(); + // Because we've already been waiting for so long we've exceeded + // our threshold and we show the next level immediately. expect(ReactNoop.getChildren()).toEqual([ span('A'), span('Loading more...'), ]); // Flush the last promise completely - Scheduler.advanceTime(4500); - await advanceTimers(4500); + Scheduler.advanceTime(5000); + await advanceTimers(5000); // Renders successfully expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['B']); expect(ReactNoop.getChildren()).toEqual([span('A'), span('B')]); }); - it('does not suspend for very long after a higher priority update', async () => { + it('does suspend if a fallback has been shown for a short time', async () => { function Foo() { Scheduler.yieldValue('Foo'); return ( }> - + + }> + + ); } - ReactNoop.discreteUpdates(() => ReactNoop.render()); + ReactNoop.render(); + // Start rendering + expect(Scheduler).toFlushAndYield([ + 'Foo', + // A suspends + 'Suspend! [A]', + // B suspends + 'Suspend! [B]', + 'Loading more...', + 'Loading...', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + + // Wait a short time. + Scheduler.advanceTime(250); + await advanceTimers(250); + expect(Scheduler).toHaveYielded(['Promise resolved [A]']); + + // Retry with the new content. + expect(Scheduler).toFlushAndYield([ + 'A', + // B still suspends + 'Suspend! [B]', + 'Loading more...', + ]); + // Because we've already been waiting for so long we can + // wait a bit longer. Still nothing... + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + + Scheduler.advanceTime(200); + await advanceTimers(200); + + // Before we commit another Promise resolves. + expect(Scheduler).toHaveYielded(['Promise resolved [B]']); + // We're still showing the first loading state. + expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); + // Restart and render the complete content. + expect(Scheduler).toFlushAndYield(['A', 'B']); + expect(ReactNoop.getChildren()).toEqual([span('A'), span('B')]); + }); + + it('does not suspend for very long after a higher priority update', async () => { + function Foo({renderContent}) { + Scheduler.yieldValue('Foo'); + return ( + }> + {renderContent ? : null} + + ); + } + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield(['Foo']); + + ReactNoop.discreteUpdates(() => + ReactNoop.render(), + ); expect(Scheduler).toFlushAndYieldThrough(['Foo']); // Advance some time. @@ -1478,6 +1619,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { it('warns when suspending inside discrete update', async () => { function A() { + Scheduler.yieldValue('A'); TextResource.read(['A', 1000]); return 'A'; } @@ -1505,13 +1647,13 @@ describe('ReactSuspenseWithNoopRenderer', () => { } ReactNoop.discreteUpdates(() => ReactNoop.render()); - Scheduler.flushAll(); + expect(Scheduler).toFlushAndYieldThrough(['A']); // Warning is not flushed until the commit phase // Timeout and commit the fallback expect(() => { - jest.advanceTimersByTime(1000); + Scheduler.flushAll(); }).toWarnDev( 'The following components suspended during a user-blocking update: A, C', {withoutStack: true}, @@ -1541,11 +1683,6 @@ describe('ReactSuspenseWithNoopRenderer', () => { 'B', 'Initial load...', ]); - // We're still suspended. - expect(ReactNoop.getChildren()).toEqual([]); - // Flush to skip suspended time. - Scheduler.advanceTime(600); - await advanceTimers(600); expect(ReactNoop.getChildren()).toEqual([span('Initial load...')]); // Eventually we resolve and show the data. @@ -1658,17 +1795,20 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); it('commits a suspended idle pri render within a reasonable time', async () => { - function Foo({something}) { + function Foo({renderContent}) { return ( }> - + {renderContent ? : null} ); } ReactNoop.render(); + expect(Scheduler).toFlushAndYield([]); + + ReactNoop.render(); // Took a long time to render. This is to ensure we get a long suspense time. // Could also use something like withSuspenseConfig to simulate this. @@ -1681,7 +1821,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Schedule an update at idle pri. Scheduler.unstable_runWithPriority(Scheduler.unstable_IdlePriority, () => - ReactNoop.render(), + ReactNoop.render(), ); expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A...']); diff --git a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap index 4372eb2baca1f..782b17b5e997d 100644 --- a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap +++ b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap @@ -363,9 +363,12 @@ exports[`ReactDebugFiberPerf supports Suspense and lazy 1`] = ` ⚛ (React Tree Reconciliation: Completed Root) ⚛ Parent [mount] - ⛔ Suspense [mount] Warning: Rendering was suspended ⚛ Suspense [mount] - ⚛ Spinner [mount] + +⚛ (Committing Changes) + ⚛ (Committing Snapshot Effects: 0 Total) + ⚛ (Committing Host Effects: 1 Total) + ⚛ (Calling Lifecycle Methods: 0 Total) " `; @@ -374,21 +377,54 @@ exports[`ReactDebugFiberPerf supports Suspense and lazy 2`] = ` ⚛ (React Tree Reconciliation: Completed Root) ⚛ Parent [mount] - ⛔ Suspense [mount] Warning: Rendering was suspended ⚛ Suspense [mount] - ⚛ Spinner [mount] + +⚛ (Committing Changes) + ⚛ (Committing Snapshot Effects: 0 Total) + ⚛ (Committing Host Effects: 1 Total) + ⚛ (Calling Lifecycle Methods: 0 Total) ⚛ (Waiting for async callback...) +⚛ (React Tree Reconciliation: Completed Root) + ⚛ Parent [update] + ⛔ Suspense [update] Warning: Rendering was suspended + ⚛ Suspense [update] + ⚛ Spinner [mount] +" +`; + +exports[`ReactDebugFiberPerf supports Suspense and lazy 3`] = ` +"⚛ (Waiting for async callback...) + ⚛ (React Tree Reconciliation: Completed Root) ⚛ Parent [mount] ⚛ Suspense [mount] - ⚛ Foo [mount] ⚛ (Committing Changes) ⚛ (Committing Snapshot Effects: 0 Total) ⚛ (Committing Host Effects: 1 Total) ⚛ (Calling Lifecycle Methods: 0 Total) + +⚛ (Waiting for async callback...) + +⚛ (React Tree Reconciliation: Completed Root) + ⚛ Parent [update] + ⛔ Suspense [update] Warning: Rendering was suspended + ⚛ Suspense [update] + ⚛ Spinner [mount] + +⚛ (Waiting for async callback...) + +⚛ (React Tree Reconciliation: Completed Root) + ⚛ Parent [update] + ⚛ Suspense [update] + ⚛ Foo [mount] + +⚛ (Committing Changes) + ⚛ (Committing Snapshot Effects: 0 Total) + ⚛ (Committing Host Effects: 2 Total) + ⚛ (Calling Lifecycle Methods: 1 Total) " `; diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 508d7814c821a..f96b5bb96ba33 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -2303,17 +2303,6 @@ describe('Profiler', () => { 'Text [Sync]', 'Monkey', ]); - // The update hasn't expired yet, so we commit nothing. - expect(ReactNoop.getChildrenAsJSX()).toEqual(null); - expect(onRender).not.toHaveBeenCalled(); - - // Advance both React's virtual time and Jest's timers by enough to expire - // the update, but not by enough to flush the suspending promise. - ReactNoop.expire(10000); - await awaitableAdvanceTimers(10000); - // No additional rendering work is required, since we already prepared - // the placeholder. - expect(Scheduler).toHaveYielded([]); // Should have committed the placeholder. expect(ReactNoop.getChildrenAsJSX()).toEqual('Loading...Sync'); expect(onRender).toHaveBeenCalledTimes(1); @@ -2333,7 +2322,7 @@ describe('Profiler', () => { expect(onRender).toHaveBeenCalledTimes(2); // Once the promise resolves, we render the suspended view - await awaitableAdvanceTimers(10000); + await awaitableAdvanceTimers(20000); expect(Scheduler).toHaveYielded(['Promise resolved [Async]']); expect(Scheduler).toFlushAndYield(['AsyncText [Async]']); expect(ReactNoop.getChildrenAsJSX()).toEqual('AsyncSync'); @@ -2633,6 +2622,16 @@ describe('Profiler', () => { }); it('handles high-pri renderers between suspended and resolved (async) trees', async () => { + // Set up an initial shell. We need to set this up before the test sceanrio + // because we want initial render to suspend on navigation to the initial state. + let renderer = ReactTestRenderer.create( + {}}> + } /> + , + {unstable_isConcurrent: true}, + ); + expect(Scheduler).toFlushAndYield([]); + const initialRenderInteraction = { id: 0, name: 'initial render', @@ -2640,19 +2639,17 @@ describe('Profiler', () => { }; const onRender = jest.fn(); - let renderer; SchedulerTracing.unstable_trace( initialRenderInteraction.name, initialRenderInteraction.timestamp, () => { - renderer = ReactTestRenderer.create( + renderer.update( }> , - {unstable_isConcurrent: true}, ); }, );