Skip to content

Commit

Permalink
Disable timeoutMs argument
Browse files Browse the repository at this point in the history 
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
acdlite committed Aug 26, 2020
1 parent 350196b commit 12efd27
Show file tree
Hide file tree
Showing 6 changed files with 211 additions and 249 deletions.
17 changes: 13 additions & 4 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Original file line number Diff line number Diff line change
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
65 changes: 23 additions & 42 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -923,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
65 changes: 23 additions & 42 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -911,60 +911,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
Loading

0 comments on commit 12efd27

Please sign in to comment.