Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Disable timeoutMs argument #19703

Merged
merged 2 commits into from
Aug 26, 2020
Merged

Disable timeoutMs argument #19703

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
350196b
Remove distinction between long, short transitions
acdlite Aug 26, 2020
93ca66a
Disable `timeoutMs` argument
acdlite Aug 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Open in desktop
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
131 changes: 40 additions & 91 deletions packages/react-reconciler/src/ReactFiberLane.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the hydration lane used for loads or is it used for something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hydration lanes are a used for this scenario:

  • During progressive hydration, only part of the initial HTML has been hydrated. Let's say a parent is hydrated, but its child has not.
  • Then, the parent receives an update (setState) and attempts to re-render the child. But the child hasn't even hydrated yet; it's still just showing the initial HTML.
  • We could render a brand new child using the newest props, but that would require discarding the initial HTML and losing its state, like text input.
  • So instead we do a clever thing: pause the parent update, then bump the priority of hydrating the child to a slightly higher priority.
  • Finish hydrating the child. It has now fully mounted.
  • Now try the parent update again and proceed like normal.

In order for the "bump the priority" trick to work, we reserve special lanes that sit in between the ranges used for regular updates. The hydration lanes only ever include this type of work, so that they don't get accidentally batched together with other updates.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See:

react/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js

Lines 506 to 575 in 60ba723

it('blocks updates to hydrate the content first if props have changed', async () => {
let suspend = false;
let resolve;
const promise = new Promise(resolvePromise => (resolve = resolvePromise));
const ref = React.createRef();
function Child({text}) {
if (suspend) {
throw promise;
} else {
return text;
}
}
function App({text, className}) {
return (
<div>
<Suspense fallback="Loading...">
<span ref={ref} className={className}>
<Child text={text} />
</span>
</Suspense>
</div>
);
}
suspend = false;
const finalHTML = ReactDOMServer.renderToString(
<App text="Hello" className="hello" />,
);
const container = document.createElement('div');
container.innerHTML = finalHTML;
const span = container.getElementsByTagName('span')[0];
// On the client we don't have all data yet but we want to start
// hydrating anyway.
suspend = true;
const root = ReactDOM.createRoot(container, {hydrate: true});
root.render(<App text="Hello" className="hello" />);
Scheduler.unstable_flushAll();
jest.runAllTimers();
expect(ref.current).toBe(null);
expect(span.textContent).toBe('Hello');
// Render an update, which will be higher or the same priority as pinging the hydration.
root.render(<App text="Hi" className="hi" />);
// At the same time, resolving the promise so that rendering can complete.
suspend = false;
resolve();
await promise;
// This should first complete the hydration and then flush the update onto the hydrated state.
Scheduler.unstable_flushAll();
jest.runAllTimers();
// The new span should be the same since we should have successfully hydrated
// before changing it.
const newSpan = container.getElementsByTagName('span')[0];
expect(span).toBe(newSpan);
// We should now have fully rendered with a ref on the new span.
expect(ref.current).toBe(span);
expect(span.textContent).toBe('Hi');
// If we ended up hydrating the existing content, we won't have properly
// patched up the tree, which might mean we haven't patched the className.
expect(span.className).toBe('hi');
});

Copy link
Member

@rickhanlonii rickhanlonii Aug 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, that makes sense. Is it possible (semantically or structurally) for hydration lanes to get entangled with non-hydration lanes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think theoretically it could happen with useMutableSource, which uses entanglement to de-opt and prevent tearing in some cases when it detects a mutation.

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
Open in desktop
Original file line number Diff line number Diff line change
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
Loading