From 8da0da0937af154b775b243c9d28b6aa50db696b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 26 Aug 2020 16:35:13 -0500 Subject: [PATCH] Disable timeoutMs argument (#19703) * Remove distinction between long, short transitions We're removing the `timeoutMs` option, so there's no longer any distinction between "short" and "long" transitions. They're all treated the same. This commit doesn't remove `timeoutMs` yet, only combines the internal priority levels. * Disable `timeoutMs` argument tl;dr ----- - We're removing the `timeoutMs` argument from `useTransition`. - Transitions will either immediately switch to a skeleton/placeholder view (when loading new content) or wait indefinitely until the data resolves (when refreshing stale content). - This commit disables the `timeoutMS` so that the API has the desired semantics. It doesn't yet update the types or migrate all the test callers. I'll do those steps in follow-up PRs. Motivation ---------- Currently, transitions initiated by `startTransition` / `useTransition` accept a `timeoutMs` option. You can use this to control the maximum amount of time that a transition is allowed to delay before we give up and show a placeholder. What we've discovered is that, in practice, every transition falls into one of two categories: a **load** or a **refresh**: - **Loading a new screen**: show the next screen as soon as possible, even if the data hasn't finished loading. Use a skeleton/placeholder UI to show progress. - **Refreshing a screen that's already visible**: keep showing the current screen indefinitely, for as long as it takes to load the fresh data, even if the current data is stale. Use a pending state (and maybe a busy indicator) to show progress. In other words, transitions should either *delay indefinitely* (for a refresh) or they should show a placeholder *instantly* (for a load). There's not much use for transitions that are delayed for a small-but-noticeable amount of time. So, the plan is to remove the `timeoutMs` option. Instead, we'll assign an effective timeout of `0` for loads, and `Infinity` for refreshes. The mechanism for distinguishing a load from a refresh already exists in the current model. If a component suspends, and the nearest Suspense boundary hasn't already mounted, we treat that as a load, because there's nothing on the screen. However, if the nearest boundary is mounted, we treat that as a refresh, since it's already showing content. If you need to fix a transition to be treated as a load instead of a refresh, or vice versa, the solution will involve rearranging the location of your Suspense boundaries. It may also involve adding a key. We're still working on proper documentation for these patterns. In the meantime, please reach out to us if you run into problems that you're unsure how to fix. We will remove `timeoutMs` from `useDeferredValue`, too, and apply the same load versus refresh semantics to the update that spawns the deferred value. Note that there are other types of delays that are not related to transitions; for example, we will still throttle the appearance of nested placeholders (we refer to this as the placeholder "train model"), and we may still apply a Just Noticeable Difference heuristic (JND) in some cases. These aren't going anywhere. (Well, the JND heuristic might but for different reasons than those discussed above.) --- .../src/__tests__/ReactTestUtilsAct-test.js | 17 +- .../react-reconciler/src/ReactFiberLane.js | 131 ++++-------- .../src/ReactFiberWorkLoop.new.js | 80 +++---- .../src/ReactFiberWorkLoop.old.js | 80 +++---- .../ReactHooksWithNoopRenderer-test.js | 118 +++++++---- .../__tests__/ReactIncrementalUpdates-test.js | 2 +- .../ReactSuspenseWithNoopRenderer-test.js | 197 +++++++----------- scripts/error-codes/codes.json | 1 - 8 files changed, 254 insertions(+), 372 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index 175813763823e..d2de57b588342 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -779,10 +779,19 @@ function runActTests(label, render, unmount, rerender) { }, {timeout: 5000}, ); - // the spinner shows up regardless - expect( - document.querySelector('[data-test-id=spinner]'), - ).not.toBeNull(); + + if (label === 'concurrent mode') { + // In Concurrent Mode, refresh transitions delay indefinitely. + expect(document.querySelector('[data-test-id=spinner]')).toBeNull(); + } else { + // In Legacy Mode and Blocking Mode, all fallbacks are forced to + // display, even during a refresh transition. + // TODO: Consider delaying indefinitely in Blocking Mode, to match + // Concurrent Mode semantics. + expect( + document.querySelector('[data-test-id=spinner]'), + ).not.toBeNull(); + } // resolve the promise await act(async () => { diff --git a/packages/react-reconciler/src/ReactFiberLane.js b/packages/react-reconciler/src/ReactFiberLane.js index d5712167714f4..d9ee9f43eb16c 100644 --- a/packages/react-reconciler/src/ReactFiberLane.js +++ b/packages/react-reconciler/src/ReactFiberLane.js @@ -43,23 +43,20 @@ import { NoPriority as NoSchedulerPriority, } from './SchedulerWithReactIntegration.new'; -export const SyncLanePriority: LanePriority = 17; -export const SyncBatchedLanePriority: LanePriority = 16; +export const SyncLanePriority: LanePriority = 15; +export const SyncBatchedLanePriority: LanePriority = 14; -const InputDiscreteHydrationLanePriority: LanePriority = 15; -export const InputDiscreteLanePriority: LanePriority = 14; +const InputDiscreteHydrationLanePriority: LanePriority = 13; +export const InputDiscreteLanePriority: LanePriority = 12; -const InputContinuousHydrationLanePriority: LanePriority = 13; -export const InputContinuousLanePriority: LanePriority = 12; +const InputContinuousHydrationLanePriority: LanePriority = 11; +export const InputContinuousLanePriority: LanePriority = 10; -const DefaultHydrationLanePriority: LanePriority = 11; -export const DefaultLanePriority: LanePriority = 10; +const DefaultHydrationLanePriority: LanePriority = 9; +export const DefaultLanePriority: LanePriority = 8; -const TransitionShortHydrationLanePriority: LanePriority = 9; -export const TransitionShortLanePriority: LanePriority = 8; - -const TransitionLongHydrationLanePriority: LanePriority = 7; -export const TransitionLongLanePriority: LanePriority = 6; +const TransitionHydrationPriority: LanePriority = 7; +export const TransitionPriority: LanePriority = 6; const RetryLanePriority: LanePriority = 5; @@ -89,11 +86,8 @@ const InputContinuousLanes: Lanes = /* */ 0b0000000000000000000 export const DefaultHydrationLane: Lane = /* */ 0b0000000000000000000000100000000; export const DefaultLanes: Lanes = /* */ 0b0000000000000000000111000000000; -const TransitionShortHydrationLane: Lane = /* */ 0b0000000000000000001000000000000; -const TransitionShortLanes: Lanes = /* */ 0b0000000000000011110000000000000; - -const TransitionLongHydrationLane: Lane = /* */ 0b0000000000000100000000000000000; -const TransitionLongLanes: Lanes = /* */ 0b0000000001111000000000000000000; +const TransitionHydrationLane: Lane = /* */ 0b0000000000000000001000000000000; +const TransitionLanes: Lanes = /* */ 0b0000000001111111110000000000000; const RetryLanes: Lanes = /* */ 0b0000011110000000000000000000000; @@ -160,23 +154,14 @@ function getHighestPriorityLanes(lanes: Lanes | Lane): Lanes { return_highestLanePriority = DefaultLanePriority; return defaultLanes; } - if ((lanes & TransitionShortHydrationLane) !== NoLanes) { - return_highestLanePriority = TransitionShortHydrationLanePriority; - return TransitionShortHydrationLane; - } - const transitionShortLanes = TransitionShortLanes & lanes; - if (transitionShortLanes !== NoLanes) { - return_highestLanePriority = TransitionShortLanePriority; - return transitionShortLanes; + if ((lanes & TransitionHydrationLane) !== NoLanes) { + return_highestLanePriority = TransitionHydrationPriority; + return TransitionHydrationLane; } - if ((lanes & TransitionLongHydrationLane) !== NoLanes) { - return_highestLanePriority = TransitionLongHydrationLanePriority; - return TransitionLongHydrationLane; - } - const transitionLongLanes = TransitionLongLanes & lanes; - if (transitionLongLanes !== NoLanes) { - return_highestLanePriority = TransitionLongLanePriority; - return transitionLongLanes; + const transitionLanes = TransitionLanes & lanes; + if (transitionLanes !== NoLanes) { + return_highestLanePriority = TransitionPriority; + return transitionLanes; } const retryLanes = RetryLanes & lanes; if (retryLanes !== NoLanes) { @@ -241,10 +226,8 @@ export function lanePriorityToSchedulerPriority( return UserBlockingSchedulerPriority; case DefaultHydrationLanePriority: case DefaultLanePriority: - case TransitionShortHydrationLanePriority: - case TransitionShortLanePriority: - case TransitionLongHydrationLanePriority: - case TransitionLongLanePriority: + case TransitionHydrationPriority: + case TransitionPriority: case SelectiveHydrationLanePriority: case RetryLanePriority: return NormalSchedulerPriority; @@ -402,7 +385,7 @@ function computeExpirationTime(lane: Lane, currentTime: number) { if (priority >= InputContinuousLanePriority) { // User interactions should expire slightly more quickly. return currentTime + 1000; - } else if (priority >= TransitionLongLanePriority) { + } else if (priority >= TransitionPriority) { return currentTime + 5000; } else { // Anything idle priority or lower should never expire. @@ -513,9 +496,7 @@ export function findUpdateLane( if (lane === NoLane) { // If all the default lanes are already being worked on, look for a // lane in the transition range. - lane = pickArbitraryLane( - (TransitionShortLanes | TransitionLongLanes) & ~wipLanes, - ); + lane = pickArbitraryLane(TransitionLanes & ~wipLanes); if (lane === NoLane) { // All the transition lanes are taken, too. This should be very // rare, but as a last resort, pick a default lane. This will have @@ -525,8 +506,7 @@ export function findUpdateLane( } return lane; } - case TransitionShortLanePriority: // Should be handled by findTransitionLane instead - case TransitionLongLanePriority: + case TransitionPriority: // Should be handled by findTransitionLane instead case RetryLanePriority: // Should be handled by findRetryLane instead break; case IdleLanePriority: @@ -548,48 +528,21 @@ export function findUpdateLane( // To ensure consistency across multiple updates in the same event, this should // be pure function, so that it always returns the same lane for given inputs. -export function findTransitionLane( - lanePriority: LanePriority, - wipLanes: Lanes, - pendingLanes: Lanes, -): Lane { - if (lanePriority === TransitionShortLanePriority) { - // First look for lanes that are completely unclaimed, i.e. have no - // pending work. - let lane = pickArbitraryLane(TransitionShortLanes & ~pendingLanes); - if (lane === NoLane) { - // If all lanes have pending work, look for a lane that isn't currently - // being worked on. - lane = pickArbitraryLane(TransitionShortLanes & ~wipLanes); - if (lane === NoLane) { - // If everything is being worked on, pick any lane. This has the - // effect of interrupting the current work-in-progress. - lane = pickArbitraryLane(TransitionShortLanes); - } - } - return lane; - } - if (lanePriority === TransitionLongLanePriority) { - // First look for lanes that are completely unclaimed, i.e. have no - // pending work. - let lane = pickArbitraryLane(TransitionLongLanes & ~pendingLanes); +export function findTransitionLane(wipLanes: Lanes, pendingLanes: Lanes): Lane { + // First look for lanes that are completely unclaimed, i.e. have no + // pending work. + let lane = pickArbitraryLane(TransitionLanes & ~pendingLanes); + if (lane === NoLane) { + // If all lanes have pending work, look for a lane that isn't currently + // being worked on. + lane = pickArbitraryLane(TransitionLanes & ~wipLanes); if (lane === NoLane) { - // If all lanes have pending work, look for a lane that isn't currently - // being worked on. - lane = pickArbitraryLane(TransitionLongLanes & ~wipLanes); - if (lane === NoLane) { - // If everything is being worked on, pick any lane. This has the - // effect of interrupting the current work-in-progress. - lane = pickArbitraryLane(TransitionLongLanes); - } + // If everything is being worked on, pick any lane. This has the + // effect of interrupting the current work-in-progress. + lane = pickArbitraryLane(TransitionLanes); } - return lane; } - invariant( - false, - 'Invalid transition priority: %s. This is a bug in React.', - lanePriority, - ); + return lane; } // To ensure consistency across multiple updates in the same event, this should @@ -816,18 +769,14 @@ export function getBumpedLaneForHydration( case DefaultLanePriority: lane = DefaultHydrationLane; break; - case TransitionShortHydrationLanePriority: - case TransitionShortLanePriority: - lane = TransitionShortHydrationLane; - break; - case TransitionLongHydrationLanePriority: - case TransitionLongLanePriority: - lane = TransitionLongHydrationLane; + case TransitionHydrationPriority: + case TransitionPriority: + lane = TransitionHydrationLane; break; case RetryLanePriority: // Shouldn't be reachable under normal circumstances, so there's no // dedicated lane for retry priority. Use the one for long transitions. - lane = TransitionLongHydrationLane; + lane = TransitionHydrationLane; break; case SelectiveHydrationLanePriority: lane = SelectiveHydrationLane; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index ecd4611c3402d..ca8f6e45b0e1c 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -153,8 +153,6 @@ import { SyncLanePriority, SyncBatchedLanePriority, InputDiscreteLanePriority, - TransitionShortLanePriority, - TransitionLongLanePriority, DefaultLanePriority, NoLanes, NoLane, @@ -457,24 +455,13 @@ export function requestUpdateLane( // Use the size of the timeout as a heuristic to prioritize shorter // transitions over longer ones. // TODO: This will coerce numbers larger than 31 bits to 0. - const timeoutMs = suspenseConfig.timeoutMs; - const transitionLanePriority = - timeoutMs === undefined || (timeoutMs | 0) < 10000 - ? TransitionShortLanePriority - : TransitionLongLanePriority; - if (currentEventPendingLanes !== NoLanes) { currentEventPendingLanes = mostRecentlyUpdatedRoot !== null ? mostRecentlyUpdatedRoot.pendingLanes : NoLanes; } - - return findTransitionLane( - transitionLanePriority, - currentEventWipLanes, - currentEventPendingLanes, - ); + return findTransitionLane(currentEventWipLanes, currentEventPendingLanes); } // TODO: Remove this dependency on the Scheduler priority. @@ -936,52 +923,32 @@ function finishConcurrentRender(root, exitStatus, lanes) { case RootSuspendedWithDelay: { markRootSuspended(root, lanes); - if ( - // do not delay if we're inside an act() scope - !shouldForceFlushFallbacksInDEV() - ) { - // 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. - const nextLanes = getNextLanes(root, NoLanes); - if (nextLanes !== NoLanes) { - // There's additional work on this root. - break; - } - const suspendedLanes = root.suspendedLanes; - if (!isSubsetOfLanes(suspendedLanes, lanes)) { - // We should prefer to render the fallback of at the last - // suspended level. Ping the last suspended level to try - // rendering it again. - // FIXME: What if the suspended lanes are Idle? Should not restart. - const eventTime = requestEventTime(); - markRootPinged(root, suspendedLanes, eventTime); - break; - } + if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) { + // This is a transition, so we should exit without committing a + // placeholder and without scheduling a timeout. Delay indefinitely + // until we receive more data. + // TODO: Check the lanes to see if it's a transition, instead of + // tracking the latest timeout. + break; + } + + if (!shouldForceFlushFallbacksInDEV()) { + // This is not a transition, but we did trigger an avoided state. + // Schedule a placeholder to display after a short delay, using the Just + // Noticable Difference. + // TODO: Is the JND optimization worth the added complexity? If this is + // the only reason we track the event time, then probably not. + // Consider removing. const mostRecentEventTime = getMostRecentEventTime(root, lanes); - let msUntilTimeout; - if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) { - // We have processed a suspense config whose expiration time we - // can use as the timeout. - msUntilTimeout = workInProgressRootLatestSuspenseTimeout - now(); - } else if (mostRecentEventTime === NoTimestamp) { - // 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 didn't process a suspense config, compute a JND based on - // the amount of time elapsed since the most recent event time. - const eventTimeMs = mostRecentEventTime; - const timeElapsedMs = now() - eventTimeMs; - msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs; - } + const eventTimeMs = mostRecentEventTime; + const timeElapsedMs = now() - eventTimeMs; + const msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs; // 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. + // Instead of committing the fallback immediately, wait for more data + // to arrive. root.timeoutHandle = scheduleTimeout( commitRoot.bind(null, root), msUntilTimeout, @@ -989,7 +956,8 @@ function finishConcurrentRender(root, exitStatus, lanes) { break; } } - // The work expired. Commit immediately. + + // Commit the placeholder. commitRoot(root); break; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 21ac3eeb65ce0..37e781a18bbb3 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -139,8 +139,6 @@ import { SyncLanePriority, SyncBatchedLanePriority, InputDiscreteLanePriority, - TransitionShortLanePriority, - TransitionLongLanePriority, DefaultLanePriority, NoLanes, NoLane, @@ -445,24 +443,13 @@ export function requestUpdateLane( // Use the size of the timeout as a heuristic to prioritize shorter // transitions over longer ones. // TODO: This will coerce numbers larger than 31 bits to 0. - const timeoutMs = suspenseConfig.timeoutMs; - const transitionLanePriority = - timeoutMs === undefined || (timeoutMs | 0) < 10000 - ? TransitionShortLanePriority - : TransitionLongLanePriority; - if (currentEventPendingLanes !== NoLanes) { currentEventPendingLanes = mostRecentlyUpdatedRoot !== null ? mostRecentlyUpdatedRoot.pendingLanes : NoLanes; } - - return findTransitionLane( - transitionLanePriority, - currentEventWipLanes, - currentEventPendingLanes, - ); + return findTransitionLane(currentEventWipLanes, currentEventPendingLanes); } // TODO: Remove this dependency on the Scheduler priority. @@ -924,52 +911,32 @@ function finishConcurrentRender(root, exitStatus, lanes) { case RootSuspendedWithDelay: { markRootSuspended(root, lanes); - if ( - // do not delay if we're inside an act() scope - !shouldForceFlushFallbacksInDEV() - ) { - // 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. - const nextLanes = getNextLanes(root, NoLanes); - if (nextLanes !== NoLanes) { - // There's additional work on this root. - break; - } - const suspendedLanes = root.suspendedLanes; - if (!isSubsetOfLanes(suspendedLanes, lanes)) { - // We should prefer to render the fallback of at the last - // suspended level. Ping the last suspended level to try - // rendering it again. - // FIXME: What if the suspended lanes are Idle? Should not restart. - const eventTime = requestEventTime(); - markRootPinged(root, suspendedLanes, eventTime); - break; - } + if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) { + // This is a transition, so we should exit without committing a + // placeholder and without scheduling a timeout. Delay indefinitely + // until we receive more data. + // TODO: Check the lanes to see if it's a transition, instead of + // tracking the latest timeout. + break; + } + + if (!shouldForceFlushFallbacksInDEV()) { + // This is not a transition, but we did trigger an avoided state. + // Schedule a placeholder to display after a short delay, using the Just + // Noticable Difference. + // TODO: Is the JND optimization worth the added complexity? If this is + // the only reason we track the event time, then probably not. + // Consider removing. const mostRecentEventTime = getMostRecentEventTime(root, lanes); - let msUntilTimeout; - if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) { - // We have processed a suspense config whose expiration time we - // can use as the timeout. - msUntilTimeout = workInProgressRootLatestSuspenseTimeout - now(); - } else if (mostRecentEventTime === NoTimestamp) { - // 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 didn't process a suspense config, compute a JND based on - // the amount of time elapsed since the most recent event time. - const eventTimeMs = mostRecentEventTime; - const timeElapsedMs = now() - eventTimeMs; - msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs; - } + const eventTimeMs = mostRecentEventTime; + const timeElapsedMs = now() - eventTimeMs; + const msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs; // 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. + // Instead of committing the fallback immediately, wait for more data + // to arrive. root.timeoutHandle = scheduleTimeout( commitRoot.bind(null, root), msUntilTimeout, @@ -977,7 +944,8 @@ function finishConcurrentRender(root, exitStatus, lanes) { break; } } - // The work expired. Commit immediately. + + // Commit the placeholder. commitRoot(root); break; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index af2d88e9e783b..8b673ed80752d 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -13,8 +13,9 @@ 'use strict'; let React; -let ReactCache; -let TextResource; +let textCache; +let readText; +let resolveText; let ReactNoop; let Scheduler; let SchedulerTracing; @@ -42,7 +43,6 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); SchedulerTracing = require('scheduler/tracing'); - ReactCache = require('react-cache'); useState = React.useState; useReducer = React.useReducer; useEffect = React.useEffect; @@ -58,27 +58,58 @@ describe('ReactHooksWithNoopRenderer', () => { Suspense = React.Suspense; act = ReactNoop.act; - TextResource = ReactCache.unstable_createResource( - ([text, ms = 0]) => { - return new Promise((resolve, reject) => - setTimeout(() => { - Scheduler.unstable_yieldValue(`Promise resolved [${text}]`); - resolve(text); - }, ms), - ); - }, - ([text, ms]) => text, - ); + textCache = new Map(); + + readText = text => { + const record = textCache.get(text); + if (record !== undefined) { + switch (record.status) { + case 'pending': + throw record.promise; + case 'rejected': + throw Error('Failed to load: ' + text); + case 'resolved': + return text; + } + } else { + let ping; + const promise = new Promise(resolve => (ping = resolve)); + const newRecord = { + status: 'pending', + ping: ping, + promise, + }; + textCache.set(text, newRecord); + throw promise; + } + }; + + resolveText = text => { + const record = textCache.get(text); + if (record !== undefined) { + if (record.status === 'pending') { + Scheduler.unstable_yieldValue(`Promise resolved [${text}]`); + record.ping(); + record.ping = null; + record.status = 'resolved'; + clearTimeout(record.promise._timer); + record.promise = null; + } + } else { + const newRecord = { + ping: null, + status: 'resolved', + promise: null, + }; + textCache.set(text, newRecord); + } + }; }); function span(prop) { return {type: 'span', hidden: false, children: [], prop}; } - function hiddenSpan(prop) { - return {type: 'span', children: [], prop, hidden: true}; - } - function Text(props) { Scheduler.unstable_yieldValue(props.text); return ; @@ -87,12 +118,17 @@ describe('ReactHooksWithNoopRenderer', () => { function AsyncText(props) { const text = props.text; try { - TextResource.read([props.text, props.ms]); + readText(text); Scheduler.unstable_yieldValue(text); return ; } catch (promise) { if (typeof promise.then === 'function') { Scheduler.unstable_yieldValue(`Suspend! [${text}]`); + if (typeof props.ms === 'number' && promise._timer === undefined) { + promise._timer = setTimeout(() => { + resolveText(text); + }, props.ms); + } } else { Scheduler.unstable_yieldValue(`Error! [${text}]`); } @@ -3235,7 +3271,7 @@ describe('ReactHooksWithNoopRenderer', () => { }> {show ? ( - + ) : ( )} @@ -3265,15 +3301,14 @@ describe('ReactHooksWithNoopRenderer', () => { Scheduler.unstable_advanceTime(500); await advanceTimers(500); - Scheduler.unstable_advanceTime(1000); - await advanceTimers(1000); + // Even after a long amount of time, we still don't show a placeholder. + Scheduler.unstable_advanceTime(100000); + await advanceTimers(100000); expect(ReactNoop.getChildren()).toEqual([ - hiddenSpan('Before... Pending: true'), - span('Loading... Pending: false'), + span('Before... Pending: true'), ]); - Scheduler.unstable_advanceTime(500); - await advanceTimers(500); + await resolveText('After... Pending: false'); expect(Scheduler).toHaveYielded([ 'Promise resolved [After... Pending: false]', ]); @@ -3283,6 +3318,7 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); }); + // @gate experimental it('delays showing loading state until after busyDelayMs + busyMinDurationMs', async () => { let transition; @@ -3301,7 +3337,7 @@ describe('ReactHooksWithNoopRenderer', () => { }> {show ? ( - + ) : ( )} @@ -3337,6 +3373,7 @@ describe('ReactHooksWithNoopRenderer', () => { // result yet. Scheduler.unstable_advanceTime(1000); await advanceTimers(1000); + await resolveText('After... Pending: false'); expect(Scheduler).toHaveYielded([ 'Promise resolved [After... Pending: false]', ]); @@ -3364,9 +3401,9 @@ describe('ReactHooksWithNoopRenderer', () => { describe('useDeferredValue', () => { // @gate experimental - it('defers text value until specified timeout', async () => { + it('defers text value', async () => { function TextBox({text}) { - return ; + return ; } let _setText; @@ -3393,8 +3430,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(Scheduler).toHaveYielded(['A', 'Suspend! [A]', 'Loading']); expect(ReactNoop.getChildren()).toEqual([span('A'), span('Loading')]); - Scheduler.unstable_advanceTime(1000); - await advanceTimers(1000); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A']); expect(ReactNoop.getChildren()).toEqual([span('A'), span('A')]); @@ -3419,22 +3455,16 @@ describe('ReactHooksWithNoopRenderer', () => { expect(Scheduler).toHaveYielded([]); expect(ReactNoop.getChildren()).toEqual([span('B'), span('A')]); - await act(async () => { - Scheduler.unstable_advanceTime(500); - await advanceTimers(500); - }); - expect(Scheduler).toHaveYielded([]); - expect(ReactNoop.getChildren()).toEqual([ - span('B'), - hiddenSpan('A'), - span('Loading'), - ]); + // Even after a long amount of time, we don't show a fallback + Scheduler.unstable_advanceTime(100000); + await advanceTimers(100000); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop.getChildren()).toEqual([span('B'), span('A')]); await act(async () => { - Scheduler.unstable_advanceTime(250); - await advanceTimers(250); + await resolveText('B'); }); - expect(Scheduler).toHaveYielded(['Promise resolved [B]', 'B']); + expect(Scheduler).toHaveYielded(['Promise resolved [B]', 'B', 'B']); expect(ReactNoop.getChildren()).toEqual([span('B'), span('B')]); }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js index 6d057d8973fb4..2dd1cd18e7c50 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js @@ -17,7 +17,7 @@ let Scheduler; // Copied from ReactFiberLanes. Don't do this! // This is hard coded directly to avoid needing to import, and // we'll remove this as we replace runWithPriority with React APIs. -const InputContinuousLanePriority = 12; +const InputContinuousLanePriority = 10; describe('ReactIncrementalUpdates', () => { beforeEach(() => { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 3ba37693491a4..693f2712f1e80 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -2355,17 +2355,11 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); expect(Scheduler).toFlushAndYield(['Suspend! [B]', 'Loading...']); - Scheduler.unstable_advanceTime(1000); - await advanceTimers(1000); - // Even after a second, we have still not yet flushed the loading state. + Scheduler.unstable_advanceTime(100000); + await advanceTimers(100000); + // Even after lots of time has passed, we have still not yet flushed the + // loading state. expect(ReactNoop.getChildren()).toEqual([span('A')]); - Scheduler.unstable_advanceTime(1100); - await advanceTimers(1100); - // After the timeout, we do show the loading state. - expect(ReactNoop.getChildren()).toEqual([ - hiddenSpan('A'), - span('Loading...'), - ]); // Later we load the data. Scheduler.unstable_advanceTime(3000); await advanceTimers(3000); @@ -2385,7 +2379,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { } return ( }> - + ); } @@ -2408,8 +2402,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // Later we load the data. - Scheduler.unstable_advanceTime(5000); - await advanceTimers(5000); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A']); expect(ReactNoop.getChildren()).toEqual([span('A')]); @@ -2422,21 +2415,14 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); expect(Scheduler).toFlushAndYield(['Suspend! [B]', 'Loading...']); - Scheduler.unstable_advanceTime(1000); - await advanceTimers(1000); - // Even after a second, we have still not yet flushed the loading state. + Scheduler.unstable_advanceTime(100000); + await advanceTimers(100000); + // Even after lots of time has passed, we have still not yet flushed the + // loading state. expect(ReactNoop.getChildren()).toEqual([span('A')]); - Scheduler.unstable_advanceTime(1100); - await advanceTimers(1100); - // After the timeout, we do show the loading state. - expect(ReactNoop.getChildren()).toEqual([ - hiddenSpan('A'), - span('Loading...'), - ]); }); // Later we load the data. - Scheduler.unstable_advanceTime(3000); - await advanceTimers(3000); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['B']); expect(ReactNoop.getChildren()).toEqual([span('B')]); @@ -2455,7 +2441,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { } return ( }> - + ); } @@ -2479,8 +2465,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // Later we load the data. - Scheduler.unstable_advanceTime(5000); - await advanceTimers(5000); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A']); expect(ReactNoop.getChildren()).toEqual([span('A')]); @@ -2493,21 +2478,14 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); expect(Scheduler).toFlushAndYield(['Suspend! [B]', 'Loading...']); - Scheduler.unstable_advanceTime(1000); - await advanceTimers(1000); - // Even after a second, we have still not yet flushed the loading state. + Scheduler.unstable_advanceTime(100000); + await advanceTimers(100000); + // Even after lots of time has passed, we have still not yet flushed the + // loading state. expect(ReactNoop.getChildren()).toEqual([span('A')]); - Scheduler.unstable_advanceTime(1100); - await advanceTimers(1100); - // After the timeout, we do show the loading state. - expect(ReactNoop.getChildren()).toEqual([ - hiddenSpan('A'), - span('Loading...'), - ]); }); // Later we load the data. - Scheduler.unstable_advanceTime(3000); - await advanceTimers(3000); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['B']); expect(ReactNoop.getChildren()).toEqual([span('B')]); @@ -2520,7 +2498,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { function App({page}) { return ( }> - + ); } @@ -2535,8 +2513,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Loading...')]); // Later we load the data. - Scheduler.unstable_advanceTime(5000); - await advanceTimers(5000); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A']); expect(ReactNoop.getChildren()).toEqual([span('A')]); @@ -2552,8 +2529,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('A')]); // Later we load the data. - Scheduler.unstable_advanceTime(3000); - await advanceTimers(3000); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['B']); expect(ReactNoop.getChildren()).toEqual([span('B')]); @@ -2562,14 +2538,11 @@ describe('ReactSuspenseWithNoopRenderer', () => { React.unstable_startTransition(() => ReactNoop.render()); expect(Scheduler).toFlushAndYield(['Suspend! [C]', 'Loading...']); - // Advance past the current (effectively) infinite timeout. - // This is enforcing temporary behavior until it's truly infinite. + // Even after lots of time has passed, we have still not yet flushed the + // loading state. Scheduler.unstable_advanceTime(100000); await advanceTimers(100000); - expect(ReactNoop.getChildren()).toEqual([ - hiddenSpan('B'), - span('Loading...'), - ]); + expect(ReactNoop.getChildren()).toEqual([span('B')]); }); // @gate experimental @@ -2583,7 +2556,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { } return ( }> - + ); } @@ -2603,8 +2576,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // Later we load the data. - Scheduler.unstable_advanceTime(5000); - await advanceTimers(5000); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['A']); expect(ReactNoop.getChildren()).toEqual([span('A')]); @@ -2623,8 +2595,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { }); // Later we load the data. - Scheduler.unstable_advanceTime(3000); - await advanceTimers(3000); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); expect(Scheduler).toFlushAndYield(['B']); expect(ReactNoop.getChildren()).toEqual([span('B')]); @@ -2635,14 +2606,11 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield(['Suspend! [C]', 'Loading...']); - // Advance past the current effectively infinite timeout. - // This is enforcing temporary behavior until it's truly infinite. + // Even after lots of time has passed, we have still not yet flushed the + // loading state. Scheduler.unstable_advanceTime(100000); await advanceTimers(100000); - expect(ReactNoop.getChildren()).toEqual([ - hiddenSpan('B'), - span('Loading...'), - ]); + expect(ReactNoop.getChildren()).toEqual([span('B')]); }); }); @@ -2711,14 +2679,11 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield(['Suspend! [C]', 'Loading...']); - // Advance past the current effectively infinite timeout. - // This is enforcing temporary behavior until it's truly infinite. + // Even after lots of time has passed, we have still not yet flushed the + // loading state. Scheduler.unstable_advanceTime(100000); await advanceTimers(100000); - expect(ReactNoop.getChildren()).toEqual([ - hiddenSpan('B'), - span('Loading...'), - ]); + expect(ReactNoop.getChildren()).toEqual([span('B')]); }); }); }); @@ -2785,21 +2750,14 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); expect(Scheduler).toFlushAndYield(['B', 'Suspend! [C]', 'Loading...']); expect(ReactNoop.getChildren()).toEqual([span('B')]); - Scheduler.unstable_advanceTime(1200); - await advanceTimers(1200); - // Even after a second, we have still not yet flushed the loading state. - expect(ReactNoop.getChildren()).toEqual([span('B')]); + // Event after a large amount of time, we never show a loading state. Scheduler.unstable_advanceTime(60000); await advanceTimers(60000); - // After the timeout we show the loading state. - expect(ReactNoop.getChildren()).toEqual([ - hiddenSpan('B'), - span('Loading...'), - ]); + expect(ReactNoop.getChildren()).toEqual([span('B')]); }); // @gate experimental - it('withSuspenseConfig timeout applies when we use an updated avoided boundary', async () => { + it('withSuspenseConfig delay applies when we use an updated avoided boundary', async () => { function App({page}) { return ( }> @@ -2807,7 +2765,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { } unstable_avoidThisFallback={true}> - + ); @@ -2816,8 +2774,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Initial render. ReactNoop.render(); expect(Scheduler).toFlushAndYield(['Hi!', 'Suspend! [A]', 'Loading...']); - Scheduler.unstable_advanceTime(3000); - await advanceTimers(3000); + await resolveText('A'); expect(Scheduler).toHaveYielded(['Promise resolved [A]']); expect(Scheduler).toFlushAndYield(['Hi!', 'A']); expect(ReactNoop.getChildren()).toEqual([span('Hi!'), span('A')]); @@ -2837,18 +2794,19 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield([]); // We should still be suspended here because this loading state should be avoided. expect(ReactNoop.getChildren()).toEqual([span('Hi!'), span('A')]); - Scheduler.unstable_advanceTime(1500); - await advanceTimers(1500); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); - expect(ReactNoop.getChildren()).toEqual([ - span('Hi!'), - hiddenSpan('A'), - span('Loading B...'), - ]); + expect(Scheduler).toFlushAndYield(['Hi!', 'B']); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); }); // @gate experimental - it('withSuspenseConfig timeout applies when we use a newly created avoided boundary', async () => { + it('withSuspenseConfig delay applies when we use a newly created avoided boundary', async () => { function App({page}) { return ( }> @@ -2859,7 +2817,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { } unstable_avoidThisFallback={true}> - + )} @@ -2885,14 +2843,21 @@ describe('ReactSuspenseWithNoopRenderer', () => { await advanceTimers(1800); expect(Scheduler).toFlushAndYield([]); // We should still be suspended here because this loading state should be avoided. - expect(ReactNoop.getChildren()).toEqual([span('Hi!'), span('A')]); - Scheduler.unstable_advanceTime(1500); - await advanceTimers(1500); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + await resolveText('B'); expect(Scheduler).toHaveYielded(['Promise resolved [B]']); - expect(ReactNoop.getChildren()).toEqual([ - span('Hi!'), - span('Loading B...'), - ]); + expect(Scheduler).toFlushAndYield(['Hi!', 'B']); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); }); // @gate experimental @@ -3934,19 +3899,21 @@ describe('ReactSuspenseWithNoopRenderer', () => { , ); - // Commit the placeholder - Scheduler.unstable_advanceTime(20000); - await advanceTimers(20000); - - expect(root).toMatchRenderedOutput( - <> -