Use performConcurrentWorkOnRoot for "sync default" (#21322)
Instead of `performSyncWorkOnRoot`. The conceptual model is that the only difference between sync default updates (in React 18) and concurrent default updates (in a future major release) is time slicing. All other behavior should be the same (i.e. the stuff in `finishConcurrentRender`). Given this, I think it makes more sense to model the implementation this way, too. This exposed a quirk in the previous implementation where non-sync work was sometimes mistaken for sync work and flushed too early. In the new implementation, `performSyncWorkOnRoot` is only used for truly synchronous renders (i.e. `SyncLane`), which should make these mistakes less common. Fixes most of the tests marked with TODOs from #21072.
Andrew Clark committed
Apr 21, 2021 at 10:29 UTC
ef37d55b68ddc45d465b84ea2ce30e8328297b2d
9 files changed
+45
-99
packages/react-reconciler/src/ReactFiberLane.new.js
+3
@@ -263,6 +263,9 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
263
// Default priority updates should not interrupt transition updates. The
264
// only difference between default updates and transition updates is that
265
// 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.
269
(nextLane === DefaultLane && (wipLane & TransitionLanes) !== NoLanes)
270
) {
271
// Keep working on the existing in-progress tree. Do not interrupt.
packages/react-reconciler/src/ReactFiberLane.old.js
+3
@@ -263,6 +263,9 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
263
// Default priority updates should not interrupt transition updates. The
264
// only difference between default updates and transition updates is that
265
// 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.
269
(nextLane === DefaultLane && (wipLane & TransitionLanes) !== NoLanes)
270
) {
271
// Keep working on the existing in-progress tree. Do not interrupt.
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+10
-27
@@ -713,16 +713,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
713
714
// Schedule a new callback.
715
let newCallbackNode;
716
- if (
717
- enableSyncDefaultUpdates &&
718
- (newCallbackPriority === DefaultLane ||
719
- newCallbackPriority === DefaultHydrationLane)
720
- ) {
721
- newCallbackNode = scheduleCallback(
722
- ImmediateSchedulerPriority,
723
- performSyncWorkOnRoot.bind(null, root),
724
- );
725
- } else if (newCallbackPriority === SyncLane) {
716
+ if (newCallbackPriority === SyncLane) {
717
// Special case: Sync React callbacks are scheduled on a special
718
// internal queue
719
scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root));
@@ -820,7 +811,13 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
811
return null;
812
}
813
823
- let exitStatus = renderRootConcurrent(root, lanes);
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);
821
if (exitStatus !== RootIncomplete) {
822
if (exitStatus === RootErrored) {
823
executionContext |= RetryAfterError;
@@ -1017,13 +1014,7 @@ function performSyncWorkOnRoot(root) {
1014
// rendering it before rendering the rest of the expired work.
1015
lanes = workInProgressRootRenderLanes;
1016
}
1020
- } else if (
1021
- !(
1022
- enableSyncDefaultUpdates &&
1023
- (includesSomeLane(lanes, DefaultLane) ||
1024
- includesSomeLane(lanes, DefaultHydrationLane))
1025
- )
1026
- ) {
1017
+ } else {
1018
// There's no remaining sync work left.
1019
ensureRootIsScheduled(root, now());
1020
return null;
@@ -1067,15 +1058,7 @@ function performSyncWorkOnRoot(root) {
1058
const finishedWork: Fiber = (root.current.alternate: any);
1059
root.finishedWork = finishedWork;
1060
root.finishedLanes = lanes;
1070
- if (
1071
- enableSyncDefaultUpdates &&
1072
- (includesSomeLane(lanes, DefaultLane) ||
1073
- includesSomeLane(lanes, DefaultHydrationLane))
1074
- ) {
1075
- finishConcurrentRender(root, exitStatus, lanes);
1076
- } else {
1077
- commitRoot(root);
1078
- }
1061
+ commitRoot(root);
1062
1063
// Before exiting, make sure there's a callback scheduled for the next
1064
// pending level.
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+10
-27
@@ -713,16 +713,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
713
714
// Schedule a new callback.
715
let newCallbackNode;
716
- if (
717
- enableSyncDefaultUpdates &&
718
- (newCallbackPriority === DefaultLane ||
719
- newCallbackPriority === DefaultHydrationLane)
720
- ) {
721
- newCallbackNode = scheduleCallback(
722
- ImmediateSchedulerPriority,
723
- performSyncWorkOnRoot.bind(null, root),
724
- );
725
- } else if (newCallbackPriority === SyncLane) {
716
+ if (newCallbackPriority === SyncLane) {
717
// Special case: Sync React callbacks are scheduled on a special
718
// internal queue
719
scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root));
@@ -820,7 +811,13 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
811
return null;
812
}
813
823
- let exitStatus = renderRootConcurrent(root, lanes);
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);
821
if (exitStatus !== RootIncomplete) {
822
if (exitStatus === RootErrored) {
823
executionContext |= RetryAfterError;
@@ -1017,13 +1014,7 @@ function performSyncWorkOnRoot(root) {
1014
// rendering it before rendering the rest of the expired work.
1015
lanes = workInProgressRootRenderLanes;
1016
}
1020
- } else if (
1021
- !(
1022
- enableSyncDefaultUpdates &&
1023
- (includesSomeLane(lanes, DefaultLane) ||
1024
- includesSomeLane(lanes, DefaultHydrationLane))
1025
- )
1026
- ) {
1017
+ } else {
1018
// There's no remaining sync work left.
1019
ensureRootIsScheduled(root, now());
1020
return null;
@@ -1067,15 +1058,7 @@ function performSyncWorkOnRoot(root) {
1058
const finishedWork: Fiber = (root.current.alternate: any);
1059
root.finishedWork = finishedWork;
1060
root.finishedLanes = lanes;
1070
- if (
1071
- enableSyncDefaultUpdates &&
1072
- (includesSomeLane(lanes, DefaultLane) ||
1073
- includesSomeLane(lanes, DefaultHydrationLane))
1074
- ) {
1075
- finishConcurrentRender(root, exitStatus, lanes);
1076
- } else {
1077
- commitRoot(root);
1078
- }
1061
+ commitRoot(root);
1062
1063
// Before exiting, make sure there's a callback scheduled for the next
1064
// pending level.
packages/react-reconciler/src/__tests__/ReactExpiration-test.js
+1
-8
@@ -440,12 +440,7 @@ describe('ReactExpiration', () => {
440
flushNextRenderIfExpired();
441
expect(Scheduler).toHaveYielded([]);
442
443
- if (gate(flags => flags.enableSyncDefaultUpdates)) {
444
- // TODO: Why is this flushed?
445
- expect(ReactNoop).toMatchRenderedOutput('Hi');
446
- } else {
447
- expect(ReactNoop).toMatchRenderedOutput(null);
448
- }
443
+ expect(ReactNoop).toMatchRenderedOutput(null);
444
445
// Advance the time some more to expire the update.
446
Scheduler.unstable_advanceTime(10000);
@@ -477,8 +472,6 @@ describe('ReactExpiration', () => {
472
// Advancing by ~5 seconds should be sufficient to expire the update. (I
473
// used a slightly larger number to allow for possible rounding.)
474
Scheduler.unstable_advanceTime(6000);
480
-
481
- ReactNoop.render('Hi');
475
flushNextRenderIfExpired();
476
expect(Scheduler).toHaveYielded([]);
477
expect(ReactNoop).toMatchRenderedOutput('Hi');
packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
+2
-11
@@ -51,11 +51,7 @@ describe('ReactFlushSync', () => {
51
// The passive effect will schedule a sync update and a normal update.
52
// They should commit in two separate batches. First the sync one.
53
expect(() => {
54
- if (gate(flags => flags.enableSyncDefaultUpdates)) {
55
- expect(Scheduler).toFlushUntilNextPaint(['1, 0', '1, 1']);
56
- } else {
57
- expect(Scheduler).toFlushUntilNextPaint(['1, 0']);
58
- }
54
+ expect(Scheduler).toFlushUntilNextPaint(['1, 0']);
55
}).toErrorDev('flushSync was called from inside a lifecycle method');
56
57
// The remaining update is not sync
@@ -63,12 +59,7 @@ describe('ReactFlushSync', () => {
59
expect(Scheduler).toHaveYielded([]);
60
61
// Now flush it.
66
- if (gate(flags => flags.enableSyncDefaultUpdates)) {
67
- // With sync default updates, passive effects are synchronously flushed.
68
- expect(Scheduler).toHaveYielded([]);
69
- } else {
70
- expect(Scheduler).toFlushUntilNextPaint(['1, 1']);
71
- }
62
+ expect(Scheduler).toFlushUntilNextPaint(['1, 1']);
63
});
64
expect(root).toMatchRenderedOutput('1, 1');
65
});
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+9
-8
@@ -1406,20 +1406,21 @@ describe('ReactHooksWithNoopRenderer', () => {
1406
setParentState(false);
1407
});
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([
1410
- // TODO: why do the children render and fire effects?
1415
'Child two render',
1416
'Child one commit',
1417
'Child two commit',
1414
- 'Parent false render',
1415
- 'Parent false commit',
1416
- ]);
1417
- } else {
1418
- expect(Scheduler).toFlushUntilNextPaint([
1419
- 'Parent false render',
1420
- 'Parent false commit',
1418
]);
1419
}
1420
+ expect(Scheduler).toFlushUntilNextPaint([
1421
+ 'Parent false render',
1422
+ 'Parent false commit',
1423
+ ]);
1424
1425
// Schedule updates for children too (which should be ignored)
1426
setChildStates.forEach(setChildState => setChildState(2));
packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js
+3
-9
@@ -62,15 +62,9 @@ describe('ReactIncrementalUpdates', () => {
62
ReactNoop.render(<Foo />);
63
expect(Scheduler).toFlushAndYieldThrough(['commit']);
64
65
- if (gate(flags => flags.enableSyncDefaultUpdates)) {
66
- // TODO: should deferredUpdates flush sync with the default update?
67
- expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
68
- expect(Scheduler).toFlushWithoutYielding();
69
- } else {
70
- expect(state).toEqual({a: 'a'});
71
- expect(Scheduler).toFlushWithoutYielding();
72
- expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
73
- }
65
+ expect(state).toEqual({a: 'a'});
66
+ expect(Scheduler).toFlushWithoutYielding();
67
+ expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
68
});
69
70
it('applies updates with equal priority in insertion order', () => {
packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
+4
-9
@@ -1569,15 +1569,10 @@ describe('useMutableSource', () => {
1569
mutateB('b0');
1570
});
1571
// Finish the current render
1572
- if (gate(flags => flags.enableSyncDefaultUpdates)) {
1573
- // Default sync will flush both without yielding
1574
- expect(Scheduler).toFlushUntilNextPaint(['c', 'a0']);
1575
- } else {
1576
- expect(Scheduler).toFlushUntilNextPaint(['c']);
1577
- // a0 will re-render because of the mutation update. But it should show
1578
- // the latest value, not the intermediate one, to avoid tearing with b.
1579
- expect(Scheduler).toFlushUntilNextPaint(['a0']);
1580
- }
1572
+ expect(Scheduler).toFlushUntilNextPaint(['c']);
1573
+ // a0 will re-render because of the mutation update. But it should show
1574
+ // the latest value, not the intermediate one, to avoid tearing with b.
1575
+ expect(Scheduler).toFlushUntilNextPaint(['a0']);
1576
1577
expect(root).toMatchRenderedOutput('a0b0c');
1578
// We should be done.