Use performConcurrentWorkOnRoot for "sync default" (#21322)

acdlite · web-flow · commit ef37d55b68dd · 2021-04-21T08:29:31.000-07:00
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.
diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js
@@ -263,6 +263,9 @@ 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.
diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js
@@ -263,6 +263,9 @@ 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.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -713,16 +713,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
 
   // Schedule a new callback.
   let newCallbackNode;
-  if (
-    enableSyncDefaultUpdates &&
-    (newCallbackPriority === DefaultLane ||
-      newCallbackPriority === DefaultHydrationLane)
-  ) {
-    newCallbackNode = scheduleCallback(
-      ImmediateSchedulerPriority,
-      performSyncWorkOnRoot.bind(null, root),
-    );
-  } else if (newCallbackPriority === SyncLane) {
+  if (newCallbackPriority === SyncLane) {
     // Special case: Sync React callbacks are scheduled on a special
     // internal queue
     scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root));
@@ -820,7 +811,13 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
     return null;
   }
 
-  let exitStatus = renderRootConcurrent(root, lanes);
+  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);
   if (exitStatus !== RootIncomplete) {
     if (exitStatus === RootErrored) {
       executionContext |= RetryAfterError;
@@ -1017,13 +1014,7 @@ function performSyncWorkOnRoot(root) {
       // rendering it before rendering the rest of the expired work.
       lanes = workInProgressRootRenderLanes;
     }
-  } else if (
-    !(
-      enableSyncDefaultUpdates &&
-      (includesSomeLane(lanes, DefaultLane) ||
-        includesSomeLane(lanes, DefaultHydrationLane))
-    )
-  ) {
+  } else {
     // There's no remaining sync work left.
     ensureRootIsScheduled(root, now());
     return null;
@@ -1067,15 +1058,7 @@ function performSyncWorkOnRoot(root) {
   const finishedWork: Fiber = (root.current.alternate: any);
   root.finishedWork = finishedWork;
   root.finishedLanes = lanes;
-  if (
-    enableSyncDefaultUpdates &&
-    (includesSomeLane(lanes, DefaultLane) ||
-      includesSomeLane(lanes, DefaultHydrationLane))
-  ) {
-    finishConcurrentRender(root, exitStatus, lanes);
-  } else {
-    commitRoot(root);
-  }
+  commitRoot(root);
 
   // Before exiting, make sure there's a callback scheduled for the next
   // pending level.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -713,16 +713,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) {
 
   // Schedule a new callback.
   let newCallbackNode;
-  if (
-    enableSyncDefaultUpdates &&
-    (newCallbackPriority === DefaultLane ||
-      newCallbackPriority === DefaultHydrationLane)
-  ) {
-    newCallbackNode = scheduleCallback(
-      ImmediateSchedulerPriority,
-      performSyncWorkOnRoot.bind(null, root),
-    );
-  } else if (newCallbackPriority === SyncLane) {
+  if (newCallbackPriority === SyncLane) {
     // Special case: Sync React callbacks are scheduled on a special
     // internal queue
     scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root));
@@ -820,7 +811,13 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
     return null;
   }
 
-  let exitStatus = renderRootConcurrent(root, lanes);
+  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);
   if (exitStatus !== RootIncomplete) {
     if (exitStatus === RootErrored) {
       executionContext |= RetryAfterError;
@@ -1017,13 +1014,7 @@ function performSyncWorkOnRoot(root) {
       // rendering it before rendering the rest of the expired work.
       lanes = workInProgressRootRenderLanes;
     }
-  } else if (
-    !(
-      enableSyncDefaultUpdates &&
-      (includesSomeLane(lanes, DefaultLane) ||
-        includesSomeLane(lanes, DefaultHydrationLane))
-    )
-  ) {
+  } else {
     // There's no remaining sync work left.
     ensureRootIsScheduled(root, now());
     return null;
@@ -1067,15 +1058,7 @@ function performSyncWorkOnRoot(root) {
   const finishedWork: Fiber = (root.current.alternate: any);
   root.finishedWork = finishedWork;
   root.finishedLanes = lanes;
-  if (
-    enableSyncDefaultUpdates &&
-    (includesSomeLane(lanes, DefaultLane) ||
-      includesSomeLane(lanes, DefaultHydrationLane))
-  ) {
-    finishConcurrentRender(root, exitStatus, lanes);
-  } else {
-    commitRoot(root);
-  }
+  commitRoot(root);
 
   // Before exiting, make sure there's a callback scheduled for the next
   // pending level.
diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js
@@ -440,12 +440,7 @@ describe('ReactExpiration', () => {
     flushNextRenderIfExpired();
     expect(Scheduler).toHaveYielded([]);
 
-    if (gate(flags => flags.enableSyncDefaultUpdates)) {
-      // TODO: Why is this flushed?
-      expect(ReactNoop).toMatchRenderedOutput('Hi');
-    } else {
-      expect(ReactNoop).toMatchRenderedOutput(null);
-    }
+    expect(ReactNoop).toMatchRenderedOutput(null);
 
     // Advance the time some more to expire the update.
     Scheduler.unstable_advanceTime(10000);
@@ -477,8 +472,6 @@ describe('ReactExpiration', () => {
     // Advancing by ~5 seconds should be sufficient to expire the update. (I
     // used a slightly larger number to allow for possible rounding.)
     Scheduler.unstable_advanceTime(6000);
-
-    ReactNoop.render('Hi');
     flushNextRenderIfExpired();
     expect(Scheduler).toHaveYielded([]);
     expect(ReactNoop).toMatchRenderedOutput('Hi');
diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
@@ -51,24 +51,15 @@ describe('ReactFlushSync', () => {
       // The passive effect will schedule a sync update and a normal update.
       // They should commit in two separate batches. First the sync one.
       expect(() => {
-        if (gate(flags => flags.enableSyncDefaultUpdates)) {
-          expect(Scheduler).toFlushUntilNextPaint(['1, 0', '1, 1']);
-        } else {
-          expect(Scheduler).toFlushUntilNextPaint(['1, 0']);
-        }
+        expect(Scheduler).toFlushUntilNextPaint(['1, 0']);
       }).toErrorDev('flushSync was called from inside a lifecycle method');
 
       // The remaining update is not sync
       ReactNoop.flushSync();
       expect(Scheduler).toHaveYielded([]);
 
       // Now flush it.
-      if (gate(flags => flags.enableSyncDefaultUpdates)) {
-        // With sync default updates, passive effects are synchronously flushed.
-        expect(Scheduler).toHaveYielded([]);
-      } else {
-        expect(Scheduler).toFlushUntilNextPaint(['1, 1']);
-      }
+      expect(Scheduler).toFlushUntilNextPaint(['1, 1']);
     });
     expect(root).toMatchRenderedOutput('1, 1');
   });
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -1406,20 +1406,21 @@ describe('ReactHooksWithNoopRenderer', () => {
           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([
-            // TODO: why do the children render and fire effects?
             'Child two render',
             'Child one commit',
             'Child two commit',
-            'Parent false render',
-            'Parent false commit',
-          ]);
-        } else {
-          expect(Scheduler).toFlushUntilNextPaint([
-            'Parent false render',
-            'Parent false commit',
           ]);
         }
+        expect(Scheduler).toFlushUntilNextPaint([
+          'Parent false render',
+          'Parent false commit',
+        ]);
 
         // Schedule updates for children too (which should be ignored)
         setChildStates.forEach(setChildState => setChildState(2));
diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js
@@ -62,15 +62,9 @@ describe('ReactIncrementalUpdates', () => {
     ReactNoop.render(<Foo />);
     expect(Scheduler).toFlushAndYieldThrough(['commit']);
 
-    if (gate(flags => flags.enableSyncDefaultUpdates)) {
-      // TODO: should deferredUpdates flush sync with the default update?
-      expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
-      expect(Scheduler).toFlushWithoutYielding();
-    } else {
-      expect(state).toEqual({a: 'a'});
-      expect(Scheduler).toFlushWithoutYielding();
-      expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
-    }
+    expect(state).toEqual({a: 'a'});
+    expect(Scheduler).toFlushWithoutYielding();
+    expect(state).toEqual({a: 'a', b: 'b', c: 'c'});
   });
 
   it('applies updates with equal priority in insertion order', () => {
diff --git a/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js b/packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
@@ -1569,15 +1569,10 @@ describe('useMutableSource', () => {
           mutateB('b0');
         });
         // Finish the current render
-        if (gate(flags => flags.enableSyncDefaultUpdates)) {
-          // Default sync will flush both without yielding
-          expect(Scheduler).toFlushUntilNextPaint(['c', 'a0']);
-        } else {
-          expect(Scheduler).toFlushUntilNextPaint(['c']);
-          // a0 will re-render because of the mutation update. But it should show
-          // the latest value, not the intermediate one, to avoid tearing with b.
-          expect(Scheduler).toFlushUntilNextPaint(['a0']);
-        }
+        expect(Scheduler).toFlushUntilNextPaint(['c']);
+        // a0 will re-render because of the mutation update. But it should show
+        // the latest value, not the intermediate one, to avoid tearing with b.
+        expect(Scheduler).toFlushUntilNextPaint(['a0']);
 
         expect(root).toMatchRenderedOutput('a0b0c');
         // We should be done.
