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.

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

Continuous updates should interrupt transitions #21323

Merged
merged 1 commit into from
Apr 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 26 additions & 3 deletions packages/react-reconciler/src/ReactFiberLane.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
enableCache,
enableSchedulingProfiler,
enableUpdaterTracking,
enableSyncDefaultUpdates,
} from 'shared/ReactFeatureFlags';
import {isDevToolsPresent} from './ReactFiberDevToolsHook.new';

Expand Down Expand Up @@ -263,16 +264,25 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
// Default priority updates should not interrupt transition updates. The
// only difference between default updates and transition updates is that
// default updates do not support refresh transitions.
// TODO: This applies to sync default updates, too. Which is probably what
// we want for default priority events, but not for continuous priority
// events like hover.
(nextLane === DefaultLane && (wipLane & TransitionLanes) !== NoLanes)
) {
// Keep working on the existing in-progress tree. Do not interrupt.
return wipLanes;
}
}

if (
// TODO: Check for root override, once that lands
enableSyncDefaultUpdates &&
(nextLanes & InputContinuousLane) !== NoLanes
) {
// When updates are sync by default, we entangle continous priority updates
// and default updates, so they render in the same batch. The only reason
// they use separate lanes is because continuous updates should interrupt
// transitions, but default updates should not.
nextLanes |= pendingLanes & DefaultLane;
}

// Check for entangled lanes and add them to the batch.
//
// A lane is said to be entangled with another when it's not allowed to render
Expand Down Expand Up @@ -467,6 +477,19 @@ export function includesOnlyTransitions(lanes: Lanes) {
return (lanes & TransitionLanes) === lanes;
}

export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) {
if (!enableSyncDefaultUpdates) {
return true;
}
const SyncDefaultLanes =
InputContinuousHydrationLane |
InputContinuousLane |
DefaultHydrationLane |
DefaultLane;
// TODO: Check for root override, once that lands
return (lanes & SyncDefaultLanes) === NoLanes;
}

export function isTransitionLane(lane: Lane) {
return (lane & TransitionLanes) !== 0;
}
Expand Down
29 changes: 26 additions & 3 deletions packages/react-reconciler/src/ReactFiberLane.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
enableCache,
enableSchedulingProfiler,
enableUpdaterTracking,
enableSyncDefaultUpdates,
} from 'shared/ReactFeatureFlags';
import {isDevToolsPresent} from './ReactFiberDevToolsHook.old';

Expand Down Expand Up @@ -263,16 +264,25 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
// Default priority updates should not interrupt transition updates. The
// only difference between default updates and transition updates is that
// default updates do not support refresh transitions.
// TODO: This applies to sync default updates, too. Which is probably what
// we want for default priority events, but not for continuous priority
// events like hover.
(nextLane === DefaultLane && (wipLane & TransitionLanes) !== NoLanes)
) {
// Keep working on the existing in-progress tree. Do not interrupt.
return wipLanes;
}
}

if (
// TODO: Check for root override, once that lands
enableSyncDefaultUpdates &&
(nextLanes & InputContinuousLane) !== NoLanes
) {
// When updates are sync by default, we entangle continous priority updates
// and default updates, so they render in the same batch. The only reason
// they use separate lanes is because continuous updates should interrupt
// transitions, but default updates should not.
nextLanes |= pendingLanes & DefaultLane;
}

// Check for entangled lanes and add them to the batch.
//
// A lane is said to be entangled with another when it's not allowed to render
Expand Down Expand Up @@ -467,6 +477,19 @@ export function includesOnlyTransitions(lanes: Lanes) {
return (lanes & TransitionLanes) === lanes;
}

export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) {
if (!enableSyncDefaultUpdates) {
return true;
}
const SyncDefaultLanes =
InputContinuousHydrationLane |
InputContinuousLane |
DefaultHydrationLane |
DefaultLane;
// TODO: Check for root override, once that lands
return (lanes & SyncDefaultLanes) === NoLanes;
}

export function isTransitionLane(lane: Lane) {
return (lane & TransitionLanes) !== 0;
}
Expand Down
31 changes: 5 additions & 26 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import {
disableSchedulerTimeoutInWorkLoop,
enableStrictEffects,
skipUnmountedBoundaries,
enableSyncDefaultUpdates,
enableUpdaterTracking,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
Expand Down Expand Up @@ -139,10 +138,6 @@ import {
NoLanes,
NoLane,
SyncLane,
DefaultLane,
DefaultHydrationLane,
InputContinuousLane,
InputContinuousHydrationLane,
NoTimestamp,
claimNextTransitionLane,
claimNextRetryLane,
Expand All @@ -154,6 +149,7 @@ import {
includesNonIdleWork,
includesOnlyRetries,
includesOnlyTransitions,
shouldTimeSlice,
getNextLanes,
markStarvedLanesAsExpired,
getLanesToRetrySynchronouslyOnError,
Expand Down Expand Up @@ -437,13 +433,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
// TODO: Move this type conversion to the event priority module.
const updateLane: Lane = (getCurrentUpdatePriority(): any);
if (updateLane !== NoLane) {
if (
enableSyncDefaultUpdates &&
(updateLane === InputContinuousLane ||
updateLane === InputContinuousHydrationLane)
) {
return DefaultLane;
}
return updateLane;
}

Expand All @@ -454,13 +443,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
// use that directly.
// TODO: Move this type conversion to the event priority module.
const eventLane: Lane = (getCurrentEventPriority(): any);
if (
enableSyncDefaultUpdates &&
(eventLane === InputContinuousLane ||
eventLane === InputContinuousHydrationLane)
) {
return DefaultLane;
}
return eventLane;
}

Expand Down Expand Up @@ -811,13 +793,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
return null;
}

let exitStatus =
enableSyncDefaultUpdates &&
(includesSomeLane(lanes, DefaultLane) ||
includesSomeLane(lanes, DefaultHydrationLane))
? // Time slicing is disabled for default updates in this root.
renderRootSync(root, lanes)
: renderRootConcurrent(root, lanes);
let exitStatus = shouldTimeSlice(root, lanes)
? renderRootConcurrent(root, lanes)
: // Time slicing is disabled for default updates in this root.
renderRootSync(root, lanes);
if (exitStatus !== RootIncomplete) {
if (exitStatus === RootErrored) {
executionContext |= RetryAfterError;
Expand Down
31 changes: 5 additions & 26 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import {
disableSchedulerTimeoutInWorkLoop,
enableStrictEffects,
skipUnmountedBoundaries,
enableSyncDefaultUpdates,
enableUpdaterTracking,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
Expand Down Expand Up @@ -139,10 +138,6 @@ import {
NoLanes,
NoLane,
SyncLane,
DefaultLane,
DefaultHydrationLane,
InputContinuousLane,
InputContinuousHydrationLane,
NoTimestamp,
claimNextTransitionLane,
claimNextRetryLane,
Expand All @@ -154,6 +149,7 @@ import {
includesNonIdleWork,
includesOnlyRetries,
includesOnlyTransitions,
shouldTimeSlice,
getNextLanes,
markStarvedLanesAsExpired,
getLanesToRetrySynchronouslyOnError,
Expand Down Expand Up @@ -437,13 +433,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
// TODO: Move this type conversion to the event priority module.
const updateLane: Lane = (getCurrentUpdatePriority(): any);
if (updateLane !== NoLane) {
if (
enableSyncDefaultUpdates &&
(updateLane === InputContinuousLane ||
updateLane === InputContinuousHydrationLane)
) {
return DefaultLane;
}
return updateLane;
}

Expand All @@ -454,13 +443,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
// use that directly.
// TODO: Move this type conversion to the event priority module.
const eventLane: Lane = (getCurrentEventPriority(): any);
if (
enableSyncDefaultUpdates &&
(eventLane === InputContinuousLane ||
eventLane === InputContinuousHydrationLane)
) {
return DefaultLane;
}
return eventLane;
}

Expand Down Expand Up @@ -811,13 +793,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
return null;
}

let exitStatus =
enableSyncDefaultUpdates &&
(includesSomeLane(lanes, DefaultLane) ||
includesSomeLane(lanes, DefaultHydrationLane))
? // Time slicing is disabled for default updates in this root.
renderRootSync(root, lanes)
: renderRootConcurrent(root, lanes);
let exitStatus = shouldTimeSlice(root, lanes)
? renderRootConcurrent(root, lanes)
: // Time slicing is disabled for default updates in this root.
renderRootSync(root, lanes);
if (exitStatus !== RootIncomplete) {
if (exitStatus === RootErrored) {
executionContext |= RetryAfterError;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1405,18 +1405,6 @@ describe('ReactHooksWithNoopRenderer', () => {
ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => {
setParentState(false);
});
if (gate(flags => flags.enableSyncDefaultUpdates)) {
// TODO: Default updates do not interrupt transition updates, to
// prevent starvation. However, when sync default updates are enabled,
// continuous updates are treated like default updates. In this case,
// we probably don't want this behavior; continuous should be allowed
// to interrupt.
expect(Scheduler).toFlushUntilNextPaint([
'Child two render',
'Child one commit',
'Child two commit',
]);
}
expect(Scheduler).toFlushUntilNextPaint([
'Parent false render',
'Parent false commit',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
let React;
let ReactNoop;
let Scheduler;
let ContinuousEventPriority;
let startTransition;
let useState;
let useEffect;

Expand All @@ -11,6 +13,9 @@ describe('ReactUpdatePriority', () => {
React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');
ContinuousEventPriority = require('react-reconciler/constants')
.ContinuousEventPriority;
startTransition = React.unstable_startTransition;
useState = React.useState;
useEffect = React.useEffect;
});
Expand Down Expand Up @@ -78,4 +83,53 @@ describe('ReactUpdatePriority', () => {
// Now the idle update has flushed
expect(Scheduler).toHaveYielded(['Idle: 2, Default: 2']);
});

// @gate experimental
test('continuous updates should interrupt transisions', async () => {
const root = ReactNoop.createRoot();

let setCounter;
let setIsHidden;
function App() {
const [counter, _setCounter] = useState(1);
const [isHidden, _setIsHidden] = useState(false);
setCounter = _setCounter;
setIsHidden = _setIsHidden;
if (isHidden) {
return <Text text={'(hidden)'} />;
}
return (
<>
<Text text={'A' + counter} />
<Text text={'B' + counter} />
<Text text={'C' + counter} />
</>
);
}

await ReactNoop.act(async () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['A1', 'B1', 'C1']);
expect(root).toMatchRenderedOutput('A1B1C1');

await ReactNoop.act(async () => {
startTransition(() => {
setCounter(2);
});
expect(Scheduler).toFlushAndYieldThrough(['A2']);
ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => {
setIsHidden(true);
});
});
expect(Scheduler).toHaveYielded([
// Because the hide update has continous priority, it should interrupt the
// in-progress transition
'(hidden)',
// When the transition resumes, it's a no-op because the children are
// now hidden.
'(hidden)',
]);
expect(root).toMatchRenderedOutput('(hidden)');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -168,18 +168,10 @@ describe('SchedulingProfiler labels', () => {
event.initEvent('mouseover', true, true);
dispatchAndSetCurrentEvent(targetRef.current, event);
});
if (gate(flags => flags.enableSyncDefaultUpdates)) {
expect(clearedMarks).toContain(
`--schedule-state-update-${formatLanes(
ReactFiberLane.DefaultLane,
)}-App`,
);
} else {
expect(clearedMarks).toContain(
`--schedule-state-update-${formatLanes(
ReactFiberLane.InputContinuousLane,
)}-App`,
);
}
expect(clearedMarks).toContain(
`--schedule-state-update-${formatLanes(
ReactFiberLane.InputContinuousLane,
)}-App`,
);
});
});