Skip to content

Commit

Permalink
Disable timeoutMs argument (facebook#19703)
Browse files Browse the repository at this point in the history 
* 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.)
  • Loading branch information
@acdlite @koto
acdlite authored and koto committed Jun 15, 2021
1 parent 30f47eb commit 898ae27
Show file tree
Hide file tree
Showing 8 changed files with 254 additions and 372 deletions.
17 changes: 13 additions & 4 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Expand Up @@ -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 () => {
Expand Down
131 changes: 40 additions & 91 deletions packages/react-reconciler/src/ReactFiberLane.js
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down
80 changes: 24 additions & 56 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Expand Up @@ -153,8 +153,6 @@ import {
SyncLanePriority,
SyncBatchedLanePriority,
InputDiscreteLanePriority,
TransitionShortLanePriority,
TransitionLongLanePriority,
DefaultLanePriority,
NoLanes,
NoLane,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -936,60 +923,41 @@ 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,
);
break;
}
}
// The work expired. Commit immediately.

// Commit the placeholder.
commitRoot(root);
break;
}
Expand Down

0 comments on commit 898ae27

Please sign in to comment.