Skip to content

Commit 12efd27

Browse files
acdliteacdlite
committed
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.)
1 parent 350196b commit 12efd27

6 files changed

Lines changed: 211 additions & 249 deletions

File tree

‎packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -779,10 +779,19 @@ function runActTests(label, render, unmount, rerender) {
779779
},
780780
{timeout: 5000},
781781
);
782-
// the spinner shows up regardless
783-
expect(
784-
document.querySelector('[data-test-id=spinner]'),
785-
).not.toBeNull();
782+
783+
if (label === 'concurrent mode') {
784+
// In Concurrent Mode, refresh transitions delay indefinitely.
785+
expect(document.querySelector('[data-test-id=spinner]')).toBeNull();
786+
} else {
787+
// In Legacy Mode and Blocking Mode, all fallbacks are forced to
788+
// display, even during a refresh transition.
789+
// TODO: Consider delaying indefinitely in Blocking Mode, to match
790+
// Concurrent Mode semantics.
791+
expect(
792+
document.querySelector('[data-test-id=spinner]'),
793+
).not.toBeNull();
794+
}
786795

787796
// resolve the promise
788797
await act(async () => {

‎packages/react-reconciler/src/ReactFiberWorkLoop.new.js‎

Lines changed: 23 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -923,60 +923,41 @@ function finishConcurrentRender(root, exitStatus, lanes) {
923923
case RootSuspendedWithDelay: {
924924
markRootSuspended(root, lanes);
925925

926-
if (
927-
// do not delay if we're inside an act() scope
928-
!shouldForceFlushFallbacksInDEV()
929-
) {
930-
// We're suspended in a state that should be avoided. We'll try to
931-
// avoid committing it for as long as the timeouts let us.
932-
const nextLanes = getNextLanes(root, NoLanes);
933-
if (nextLanes !== NoLanes) {
934-
// There's additional work on this root.
935-
break;
936-
}
937-
const suspendedLanes = root.suspendedLanes;
938-
if (!isSubsetOfLanes(suspendedLanes, lanes)) {
939-
// We should prefer to render the fallback of at the last
940-
// suspended level. Ping the last suspended level to try
941-
// rendering it again.
942-
// FIXME: What if the suspended lanes are Idle? Should not restart.
943-
const eventTime = requestEventTime();
944-
markRootPinged(root, suspendedLanes, eventTime);
945-
break;
946-
}
926+
if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) {
927+
// This is a transition, so we should exit without committing a
928+
// placeholder and without scheduling a timeout. Delay indefinitely
929+
// until we receive more data.
930+
// TODO: Check the lanes to see if it's a transition, instead of
931+
// tracking the latest timeout.
932+
break;
933+
}
934+
935+
if (!shouldForceFlushFallbacksInDEV()) {
936+
// This is not a transition, but we did trigger an avoided state.
937+
// Schedule a placeholder to display after a short delay, using the Just
938+
// Noticable Difference.
939+
// TODO: Is the JND optimization worth the added complexity? If this is
940+
// the only reason we track the event time, then probably not.
941+
// Consider removing.
947942

948943
const mostRecentEventTime = getMostRecentEventTime(root, lanes);
949-
let msUntilTimeout;
950-
if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) {
951-
// We have processed a suspense config whose expiration time we
952-
// can use as the timeout.
953-
msUntilTimeout = workInProgressRootLatestSuspenseTimeout - now();
954-
} else if (mostRecentEventTime === NoTimestamp) {
955-
// This should never normally happen because only new updates
956-
// cause delayed states, so we should have processed something.
957-
// However, this could also happen in an offscreen tree.
958-
msUntilTimeout = 0;
959-
} else {
960-
// If we didn't process a suspense config, compute a JND based on
961-
// the amount of time elapsed since the most recent event time.
962-
const eventTimeMs = mostRecentEventTime;
963-
const timeElapsedMs = now() - eventTimeMs;
964-
msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs;
965-
}
944+
const eventTimeMs = mostRecentEventTime;
945+
const timeElapsedMs = now() - eventTimeMs;
946+
const msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs;
966947

967948
// Don't bother with a very short suspense time.
968949
if (msUntilTimeout > 10) {
969-
// The render is suspended, it hasn't timed out, and there's no
970-
// lower priority work to do. Instead of committing the fallback
971-
// immediately, wait for more data to arrive.
950+
// Instead of committing the fallback immediately, wait for more data
951+
// to arrive.
972952
root.timeoutHandle = scheduleTimeout(
973953
commitRoot.bind(null, root),
974954
msUntilTimeout,
975955
);
976956
break;
977957
}
978958
}
979-
// The work expired. Commit immediately.
959+
960+
// Commit the placeholder.
980961
commitRoot(root);
981962
break;
982963
}

‎packages/react-reconciler/src/ReactFiberWorkLoop.old.js‎

Lines changed: 23 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -911,60 +911,41 @@ function finishConcurrentRender(root, exitStatus, lanes) {
911911
case RootSuspendedWithDelay: {
912912
markRootSuspended(root, lanes);
913913

914-
if (
915-
// do not delay if we're inside an act() scope
916-
!shouldForceFlushFallbacksInDEV()
917-
) {
918-
// We're suspended in a state that should be avoided. We'll try to
919-
// avoid committing it for as long as the timeouts let us.
920-
const nextLanes = getNextLanes(root, NoLanes);
921-
if (nextLanes !== NoLanes) {
922-
// There's additional work on this root.
923-
break;
924-
}
925-
const suspendedLanes = root.suspendedLanes;
926-
if (!isSubsetOfLanes(suspendedLanes, lanes)) {
927-
// We should prefer to render the fallback of at the last
928-
// suspended level. Ping the last suspended level to try
929-
// rendering it again.
930-
// FIXME: What if the suspended lanes are Idle? Should not restart.
931-
const eventTime = requestEventTime();
932-
markRootPinged(root, suspendedLanes, eventTime);
933-
break;
934-
}
914+
if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) {
915+
// This is a transition, so we should exit without committing a
916+
// placeholder and without scheduling a timeout. Delay indefinitely
917+
// until we receive more data.
918+
// TODO: Check the lanes to see if it's a transition, instead of
919+
// tracking the latest timeout.
920+
break;
921+
}
922+
923+
if (!shouldForceFlushFallbacksInDEV()) {
924+
// This is not a transition, but we did trigger an avoided state.
925+
// Schedule a placeholder to display after a short delay, using the Just
926+
// Noticable Difference.
927+
// TODO: Is the JND optimization worth the added complexity? If this is
928+
// the only reason we track the event time, then probably not.
929+
// Consider removing.
935930

936931
const mostRecentEventTime = getMostRecentEventTime(root, lanes);
937-
let msUntilTimeout;
938-
if (workInProgressRootLatestSuspenseTimeout !== NoTimestamp) {
939-
// We have processed a suspense config whose expiration time we
940-
// can use as the timeout.
941-
msUntilTimeout = workInProgressRootLatestSuspenseTimeout - now();
942-
} else if (mostRecentEventTime === NoTimestamp) {
943-
// This should never normally happen because only new updates
944-
// cause delayed states, so we should have processed something.
945-
// However, this could also happen in an offscreen tree.
946-
msUntilTimeout = 0;
947-
} else {
948-
// If we didn't process a suspense config, compute a JND based on
949-
// the amount of time elapsed since the most recent event time.
950-
const eventTimeMs = mostRecentEventTime;
951-
const timeElapsedMs = now() - eventTimeMs;
952-
msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs;
953-
}
932+
const eventTimeMs = mostRecentEventTime;
933+
const timeElapsedMs = now() - eventTimeMs;
934+
const msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs;
954935

955936
// Don't bother with a very short suspense time.
956937
if (msUntilTimeout > 10) {
957-
// The render is suspended, it hasn't timed out, and there's no
958-
// lower priority work to do. Instead of committing the fallback
959-
// immediately, wait for more data to arrive.
938+
// Instead of committing the fallback immediately, wait for more data
939+
// to arrive.
960940
root.timeoutHandle = scheduleTimeout(
961941
commitRoot.bind(null, root),
962942
msUntilTimeout,
963943
);
964944
break;
965945
}
966946
}
967-
// The work expired. Commit immediately.
947+
948+
// Commit the placeholder.
968949
commitRoot(root);
969950
break;
970951
}

0 commit comments

Comments
 (0)