Skip to content

Commit 89847bf

Browse files
acdliteacdlite
authored
Continuous updates should interrupt transitions (#21323)
Even when updates are sync by default. Discovered this quirk while working on #21322. Previously, when sync default updates are enabled, continuous updates are treated like default updates. We implemented this by assigning DefaultLane to continous updates. However, an unintended consequence of that approach is that continuous updates would no longer interrupt transitions, because default updates are not supposed to interrupt transitions. To fix this, I changed the implementation to always assign separate lanes for default and continuous updates. Then I entangle the lanes together.
1 parent ef37d55 commit 89847bf

File tree

7 files changed

+121
-83
lines changed

7 files changed

+121
-83
lines changed

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

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import {
3939
enableCache,
4040
enableSchedulingProfiler,
4141
enableUpdaterTracking,
42+
enableSyncDefaultUpdates,
4243
} from 'shared/ReactFeatureFlags';
4344
import {isDevToolsPresent} from './ReactFiberDevToolsHook.new';
4445

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

274+
if (
275+
// TODO: Check for root override, once that lands
276+
enableSyncDefaultUpdates &&
277+
(nextLanes & InputContinuousLane) !== NoLanes
278+
) {
279+
// When updates are sync by default, we entangle continous priority updates
280+
// and default updates, so they render in the same batch. The only reason
281+
// they use separate lanes is because continuous updates should interrupt
282+
// transitions, but default updates should not.
283+
nextLanes |= pendingLanes & DefaultLane;
284+
}
285+
276286
// Check for entangled lanes and add them to the batch.
277287
//
278288
// A lane is said to be entangled with another when it's not allowed to render
@@ -467,6 +477,19 @@ export function includesOnlyTransitions(lanes: Lanes) {
467477
return (lanes & TransitionLanes) === lanes;
468478
}
469479

480+
export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) {
481+
if (!enableSyncDefaultUpdates) {
482+
return true;
483+
}
484+
const SyncDefaultLanes =
485+
InputContinuousHydrationLane |
486+
InputContinuousLane |
487+
DefaultHydrationLane |
488+
DefaultLane;
489+
// TODO: Check for root override, once that lands
490+
return (lanes & SyncDefaultLanes) === NoLanes;
491+
}
492+
470493
export function isTransitionLane(lane: Lane) {
471494
return (lane & TransitionLanes) !== 0;
472495
}

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

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import {
3939
enableCache,
4040
enableSchedulingProfiler,
4141
enableUpdaterTracking,
42+
enableSyncDefaultUpdates,
4243
} from 'shared/ReactFeatureFlags';
4344
import {isDevToolsPresent} from './ReactFiberDevToolsHook.old';
4445

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

274+
if (
275+
// TODO: Check for root override, once that lands
276+
enableSyncDefaultUpdates &&
277+
(nextLanes & InputContinuousLane) !== NoLanes
278+
) {
279+
// When updates are sync by default, we entangle continous priority updates
280+
// and default updates, so they render in the same batch. The only reason
281+
// they use separate lanes is because continuous updates should interrupt
282+
// transitions, but default updates should not.
283+
nextLanes |= pendingLanes & DefaultLane;
284+
}
285+
276286
// Check for entangled lanes and add them to the batch.
277287
//
278288
// A lane is said to be entangled with another when it's not allowed to render
@@ -467,6 +477,19 @@ export function includesOnlyTransitions(lanes: Lanes) {
467477
return (lanes & TransitionLanes) === lanes;
468478
}
469479

480+
export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) {
481+
if (!enableSyncDefaultUpdates) {
482+
return true;
483+
}
484+
const SyncDefaultLanes =
485+
InputContinuousHydrationLane |
486+
InputContinuousLane |
487+
DefaultHydrationLane |
488+
DefaultLane;
489+
// TODO: Check for root override, once that lands
490+
return (lanes & SyncDefaultLanes) === NoLanes;
491+
}
492+
470493
export function isTransitionLane(lane: Lane) {
471494
return (lane & TransitionLanes) !== 0;
472495
}

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

Lines changed: 5 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ import {
3232
disableSchedulerTimeoutInWorkLoop,
3333
enableStrictEffects,
3434
skipUnmountedBoundaries,
35-
enableSyncDefaultUpdates,
3635
enableUpdaterTracking,
3736
} from 'shared/ReactFeatureFlags';
3837
import ReactSharedInternals from 'shared/ReactSharedInternals';
@@ -139,10 +138,6 @@ import {
139138
NoLanes,
140139
NoLane,
141140
SyncLane,
142-
DefaultLane,
143-
DefaultHydrationLane,
144-
InputContinuousLane,
145-
InputContinuousHydrationLane,
146141
NoTimestamp,
147142
claimNextTransitionLane,
148143
claimNextRetryLane,
@@ -154,6 +149,7 @@ import {
154149
includesNonIdleWork,
155150
includesOnlyRetries,
156151
includesOnlyTransitions,
152+
shouldTimeSlice,
157153
getNextLanes,
158154
markStarvedLanesAsExpired,
159155
getLanesToRetrySynchronouslyOnError,
@@ -437,13 +433,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
437433
// TODO: Move this type conversion to the event priority module.
438434
const updateLane: Lane = (getCurrentUpdatePriority(): any);
439435
if (updateLane !== NoLane) {
440-
if (
441-
enableSyncDefaultUpdates &&
442-
(updateLane === InputContinuousLane ||
443-
updateLane === InputContinuousHydrationLane)
444-
) {
445-
return DefaultLane;
446-
}
447436
return updateLane;
448437
}
449438

@@ -454,13 +443,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
454443
// use that directly.
455444
// TODO: Move this type conversion to the event priority module.
456445
const eventLane: Lane = (getCurrentEventPriority(): any);
457-
if (
458-
enableSyncDefaultUpdates &&
459-
(eventLane === InputContinuousLane ||
460-
eventLane === InputContinuousHydrationLane)
461-
) {
462-
return DefaultLane;
463-
}
464446
return eventLane;
465447
}
466448

@@ -811,13 +793,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
811793
return null;
812794
}
813795

814-
let exitStatus =
815-
enableSyncDefaultUpdates &&
816-
(includesSomeLane(lanes, DefaultLane) ||
817-
includesSomeLane(lanes, DefaultHydrationLane))
818-
? // Time slicing is disabled for default updates in this root.
819-
renderRootSync(root, lanes)
820-
: renderRootConcurrent(root, lanes);
796+
let exitStatus = shouldTimeSlice(root, lanes)
797+
? renderRootConcurrent(root, lanes)
798+
: // Time slicing is disabled for default updates in this root.
799+
renderRootSync(root, lanes);
821800
if (exitStatus !== RootIncomplete) {
822801
if (exitStatus === RootErrored) {
823802
executionContext |= RetryAfterError;

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

Lines changed: 5 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ import {
3232
disableSchedulerTimeoutInWorkLoop,
3333
enableStrictEffects,
3434
skipUnmountedBoundaries,
35-
enableSyncDefaultUpdates,
3635
enableUpdaterTracking,
3736
} from 'shared/ReactFeatureFlags';
3837
import ReactSharedInternals from 'shared/ReactSharedInternals';
@@ -139,10 +138,6 @@ import {
139138
NoLanes,
140139
NoLane,
141140
SyncLane,
142-
DefaultLane,
143-
DefaultHydrationLane,
144-
InputContinuousLane,
145-
InputContinuousHydrationLane,
146141
NoTimestamp,
147142
claimNextTransitionLane,
148143
claimNextRetryLane,
@@ -154,6 +149,7 @@ import {
154149
includesNonIdleWork,
155150
includesOnlyRetries,
156151
includesOnlyTransitions,
152+
shouldTimeSlice,
157153
getNextLanes,
158154
markStarvedLanesAsExpired,
159155
getLanesToRetrySynchronouslyOnError,
@@ -437,13 +433,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
437433
// TODO: Move this type conversion to the event priority module.
438434
const updateLane: Lane = (getCurrentUpdatePriority(): any);
439435
if (updateLane !== NoLane) {
440-
if (
441-
enableSyncDefaultUpdates &&
442-
(updateLane === InputContinuousLane ||
443-
updateLane === InputContinuousHydrationLane)
444-
) {
445-
return DefaultLane;
446-
}
447436
return updateLane;
448437
}
449438

@@ -454,13 +443,6 @@ export function requestUpdateLane(fiber: Fiber): Lane {
454443
// use that directly.
455444
// TODO: Move this type conversion to the event priority module.
456445
const eventLane: Lane = (getCurrentEventPriority(): any);
457-
if (
458-
enableSyncDefaultUpdates &&
459-
(eventLane === InputContinuousLane ||
460-
eventLane === InputContinuousHydrationLane)
461-
) {
462-
return DefaultLane;
463-
}
464446
return eventLane;
465447
}
466448

@@ -811,13 +793,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
811793
return null;
812794
}
813795

814-
let exitStatus =
815-
enableSyncDefaultUpdates &&
816-
(includesSomeLane(lanes, DefaultLane) ||
817-
includesSomeLane(lanes, DefaultHydrationLane))
818-
? // Time slicing is disabled for default updates in this root.
819-
renderRootSync(root, lanes)
820-
: renderRootConcurrent(root, lanes);
796+
let exitStatus = shouldTimeSlice(root, lanes)
797+
? renderRootConcurrent(root, lanes)
798+
: // Time slicing is disabled for default updates in this root.
799+
renderRootSync(root, lanes);
821800
if (exitStatus !== RootIncomplete) {
822801
if (exitStatus === RootErrored) {
823802
executionContext |= RetryAfterError;

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1405,18 +1405,6 @@ describe('ReactHooksWithNoopRenderer', () => {
14051405
ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => {
14061406
setParentState(false);
14071407
});
1408-
if (gate(flags => flags.enableSyncDefaultUpdates)) {
1409-
// TODO: Default updates do not interrupt transition updates, to
1410-
// prevent starvation. However, when sync default updates are enabled,
1411-
// continuous updates are treated like default updates. In this case,
1412-
// we probably don't want this behavior; continuous should be allowed
1413-
// to interrupt.
1414-
expect(Scheduler).toFlushUntilNextPaint([
1415-
'Child two render',
1416-
'Child one commit',
1417-
'Child two commit',
1418-
]);
1419-
}
14201408
expect(Scheduler).toFlushUntilNextPaint([
14211409
'Parent false render',
14221410
'Parent false commit',

‎packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
let React;
22
let ReactNoop;
33
let Scheduler;
4+
let ContinuousEventPriority;
5+
let startTransition;
46
let useState;
57
let useEffect;
68

@@ -11,6 +13,9 @@ describe('ReactUpdatePriority', () => {
1113
React = require('react');
1214
ReactNoop = require('react-noop-renderer');
1315
Scheduler = require('scheduler');
16+
ContinuousEventPriority = require('react-reconciler/constants')
17+
.ContinuousEventPriority;
18+
startTransition = React.unstable_startTransition;
1419
useState = React.useState;
1520
useEffect = React.useEffect;
1621
});
@@ -78,4 +83,53 @@ describe('ReactUpdatePriority', () => {
7883
// Now the idle update has flushed
7984
expect(Scheduler).toHaveYielded(['Idle: 2, Default: 2']);
8085
});
86+
87+
// @gate experimental
88+
test('continuous updates should interrupt transisions', async () => {
89+
const root = ReactNoop.createRoot();
90+
91+
let setCounter;
92+
let setIsHidden;
93+
function App() {
94+
const [counter, _setCounter] = useState(1);
95+
const [isHidden, _setIsHidden] = useState(false);
96+
setCounter = _setCounter;
97+
setIsHidden = _setIsHidden;
98+
if (isHidden) {
99+
return <Text text={'(hidden)'} />;
100+
}
101+
return (
102+
<>
103+
<Text text={'A' + counter} />
104+
<Text text={'B' + counter} />
105+
<Text text={'C' + counter} />
106+
</>
107+
);
108+
}
109+
110+
await ReactNoop.act(async () => {
111+
root.render(<App />);
112+
});
113+
expect(Scheduler).toHaveYielded(['A1', 'B1', 'C1']);
114+
expect(root).toMatchRenderedOutput('A1B1C1');
115+
116+
await ReactNoop.act(async () => {
117+
startTransition(() => {
118+
setCounter(2);
119+
});
120+
expect(Scheduler).toFlushAndYieldThrough(['A2']);
121+
ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => {
122+
setIsHidden(true);
123+
});
124+
});
125+
expect(Scheduler).toHaveYielded([
126+
// Because the hide update has continous priority, it should interrupt the
127+
// in-progress transition
128+
'(hidden)',
129+
// When the transition resumes, it's a no-op because the children are
130+
// now hidden.
131+
'(hidden)',
132+
]);
133+
expect(root).toMatchRenderedOutput('(hidden)');
134+
});
81135
});

‎packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -168,18 +168,10 @@ describe('SchedulingProfiler labels', () => {
168168
event.initEvent('mouseover', true, true);
169169
dispatchAndSetCurrentEvent(targetRef.current, event);
170170
});
171-
if (gate(flags => flags.enableSyncDefaultUpdates)) {
172-
expect(clearedMarks).toContain(
173-
`--schedule-state-update-${formatLanes(
174-
ReactFiberLane.DefaultLane,
175-
)}-App`,
176-
);
177-
} else {
178-
expect(clearedMarks).toContain(
179-
`--schedule-state-update-${formatLanes(
180-
ReactFiberLane.InputContinuousLane,
181-
)}-App`,
182-
);
183-
}
171+
expect(clearedMarks).toContain(
172+
`--schedule-state-update-${formatLanes(
173+
ReactFiberLane.InputContinuousLane,
174+
)}-App`,
175+
);
184176
});
185177
});

0 commit comments

Comments
 (0)