Schedule sync updates in microtask (facebook#20872)

* Schedule sync updates in microtask * Updates from review * Fix comment
koto · Jun 15, 2021 · 29120d3 · 29120d3
1 parent 9b4dc9c
commit 29120d3
Show file tree

Hide file tree

Showing 23 changed files with 121 additions and 51 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
@@ -380,6 +380,7 @@ describe('ReactDOMServerPartialHydration', () => {
     resolve();
     await promise;
     Scheduler.unstable_flushAll();
+    await null;
     jest.runAllTimers();
 
     // We should now have hydrated with a ref on the existing span.

diff --git a/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js b/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js
@@ -488,6 +488,7 @@ describe('ReactDOMServerHydration', () => {
     jest.runAllTimers();
     await Promise.resolve();
     Scheduler.unstable_flushAll();
+    await null;
     expect(element.textContent).toBe('Hello world');
   });
 

diff --git a/packages/react-reconciler/src/SchedulerWithReactIntegration.new.js b/packages/react-reconciler/src/SchedulerWithReactIntegration.new.js
@@ -16,13 +16,15 @@ import {__interactionsRef} from 'scheduler/tracing';
 import {
   enableSchedulerTracing,
   decoupleUpdatePriorityFromScheduler,
+  enableSyncMicroTasks,
 } from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import {
   SyncLanePriority,
   getCurrentUpdateLanePriority,
   setCurrentUpdateLanePriority,
 } from './ReactFiberLane.new';
+import {scheduleMicrotask, supportsMicrotasks} from './ReactFiberHostConfig';
 
 const {
   unstable_runWithPriority: Scheduler_runWithPriority,
@@ -144,13 +146,19 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
   // the next tick, or earlier if something calls `flushSyncCallbackQueue`.
   if (syncQueue === null) {
     syncQueue = [callback];
-    // Flush the queue in the next tick, at the earliest.
+
     // TODO: Figure out how to remove this It's only here as a last resort if we
     // forget to explicitly flush.
-    immediateQueueCallbackNode = Scheduler_scheduleCallback(
-      Scheduler_ImmediatePriority,
-      flushSyncCallbackQueueImpl,
-    );
+    if (enableSyncMicroTasks && supportsMicrotasks) {
+      // Flush the queue in a microtask.
+      scheduleMicrotask(flushSyncCallbackQueueImpl);
+    } else {
+      // Flush the queue in the next tick.
+      immediateQueueCallbackNode = Scheduler_scheduleCallback(
+        Scheduler_ImmediatePriority,
+        flushSyncCallbackQueueImpl,
+      );
+    }
   } else {
     // Push onto existing queue. Don't need to schedule a callback because
     // we already scheduled one when we created the queue.

diff --git a/packages/react-reconciler/src/SchedulerWithReactIntegration.old.js b/packages/react-reconciler/src/SchedulerWithReactIntegration.old.js
@@ -16,13 +16,15 @@ import {__interactionsRef} from 'scheduler/tracing';
 import {
   enableSchedulerTracing,
   decoupleUpdatePriorityFromScheduler,
+  enableSyncMicroTasks,
 } from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import {
   SyncLanePriority,
   getCurrentUpdateLanePriority,
   setCurrentUpdateLanePriority,
 } from './ReactFiberLane.old';
+import {scheduleMicrotask, supportsMicrotasks} from './ReactFiberHostConfig';
 
 const {
   unstable_runWithPriority: Scheduler_runWithPriority,
@@ -144,13 +146,19 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
   // the next tick, or earlier if something calls `flushSyncCallbackQueue`.
   if (syncQueue === null) {
     syncQueue = [callback];
-    // Flush the queue in the next tick, at the earliest.
+
     // TODO: Figure out how to remove this It's only here as a last resort if we
     // forget to explicitly flush.
-    immediateQueueCallbackNode = Scheduler_scheduleCallback(
-      Scheduler_ImmediatePriority,
-      flushSyncCallbackQueueImpl,
-    );
+    if (enableSyncMicroTasks && supportsMicrotasks) {
+      // Flush the queue in a microtask.
+      scheduleMicrotask(flushSyncCallbackQueueImpl);
+    } else {
+      // Flush the queue in the next tick.
+      immediateQueueCallbackNode = Scheduler_scheduleCallback(
+        Scheduler_ImmediatePriority,
+        flushSyncCallbackQueueImpl,
+      );
+    }
   } else {
     // Push onto existing queue. Don't need to schedule a callback because
     // we already scheduled one when we created the queue.

diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js
@@ -598,12 +598,13 @@ describe('ReactExpiration', () => {
       // second one.
       Scheduler.unstable_advanceTime(1000);
       // Attempt to interrupt with a high pri update.
-      updateHighPri();
+      await ReactNoop.act(async () => {
+        updateHighPri();
+      });
 
-      // The first update expired, so first will finish it without
-      // interrupting. But not the second update, which hasn't expired yet.
-      expect(Scheduler).toFlushExpired(['Sibling']);
-      expect(Scheduler).toFlushAndYield([
+      expect(Scheduler).toHaveYielded([
+        // The first update expired
+        'Sibling',
         // Then render the high pri update
         'High pri: 1',
         'Normal pri: 1',

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -1792,7 +1792,7 @@ describe('ReactHooksWithNoopRenderer', () => {
     it(
       'in legacy mode, useEffect is deferred and updates finish synchronously ' +
         '(in a single batch)',
-      () => {
+      async () => {
         function Counter(props) {
           const [count, updateCount] = useState('(empty)');
           useEffect(() => {
@@ -1807,10 +1807,12 @@ describe('ReactHooksWithNoopRenderer', () => {
           }, [props.count]);
           return <Text text={'Count: ' + count} />;
         }
-        act(() => {
+        await act(async () => {
           ReactNoop.renderLegacySyncRoot(<Counter count={0} />);
+
           // Even in legacy mode, effects are deferred until after paint
-          expect(Scheduler).toFlushAndYieldThrough(['Count: (empty)']);
+          ReactNoop.flushSync();
+          expect(Scheduler).toHaveYielded(['Count: (empty)']);
           expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);
         });
 

diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
@@ -1400,6 +1400,10 @@ describe('ReactIncrementalErrorHandling', () => {
       'BrokenRenderAndUnmount componentWillUnmount',
     ]);
     expect(ReactNoop.getChildren()).toEqual([]);
+
+    expect(() => {
+      ReactNoop.flushSync();
+    }).toThrow('One does not simply unmount me.');
   });
 
   it('does not interrupt unmounting if detaching a ref throws', () => {

diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -98,8 +98,11 @@ describe('ReactOffscreen', () => {
           <Text text="Outside" />
         </>,
       );
+
+      ReactNoop.flushSync();
+
       // Should not defer the hidden tree
-      expect(Scheduler).toFlushUntilNextPaint(['A', 'Outside']);
+      expect(Scheduler).toHaveYielded(['A', 'Outside']);
     });
     expect(root).toMatchRenderedOutput(
       <>

diff --git a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js
@@ -569,9 +569,11 @@ describe(
           ReactNoop.render(<App />);
         });
 
+        ReactNoop.flushSync();
+
         // Because the render expired, React should finish the tree without
         // consulting `shouldYield` again
-        expect(Scheduler).toFlushExpired(['B', 'C']);
+        expect(Scheduler).toHaveYielded(['B', 'C']);
       });
     });
   },

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseList-test.js
@@ -292,9 +292,11 @@ describe('ReactSuspenseList', () => {
       </>,
     );
 
-    await C.resolve();
+    await ReactNoop.act(async () => {
+      C.resolve();
+    });
 
-    expect(Scheduler).toFlushAndYield(['C']);
+    expect(Scheduler).toHaveYielded(['C']);
 
     expect(ReactNoop).toMatchRenderedOutput(
       <>
@@ -304,9 +306,11 @@ describe('ReactSuspenseList', () => {
       </>,
     );
 
-    await B.resolve();
+    await ReactNoop.act(async () => {
+      B.resolve();
+    });
 
-    expect(Scheduler).toFlushAndYield(['B']);
+    expect(Scheduler).toHaveYielded(['B']);
 
     expect(ReactNoop).toMatchRenderedOutput(
       <>

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js
@@ -310,7 +310,7 @@ describe('ReactSuspensePlaceholder', () => {
     });
 
     describe('when suspending during mount', () => {
-      it('properly accounts for base durations when a suspended times out in a legacy tree', () => {
+      it('properly accounts for base durations when a suspended times out in a legacy tree', async () => {
         ReactNoop.renderLegacySyncRoot(<App shouldSuspend={true} />);
         expect(Scheduler).toHaveYielded([
           'App',
@@ -331,7 +331,10 @@ describe('ReactSuspensePlaceholder', () => {
         jest.advanceTimersByTime(1000);
 
         expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']);
-        expect(Scheduler).toFlushExpired(['Loaded']);
+
+        ReactNoop.flushSync();
+
+        expect(Scheduler).toHaveYielded(['Loaded']);
         expect(ReactNoop).toMatchRenderedOutput('LoadedText');
         expect(onRender).toHaveBeenCalledTimes(2);
 
@@ -378,7 +381,7 @@ describe('ReactSuspensePlaceholder', () => {
     });
 
     describe('when suspending during update', () => {
-      it('properly accounts for base durations when a suspended times out in a legacy tree', () => {
+      it('properly accounts for base durations when a suspended times out in a legacy tree', async () => {
         ReactNoop.renderLegacySyncRoot(
           <App shouldSuspend={false} textRenderDuration={5} />,
         );
@@ -427,7 +430,10 @@ describe('ReactSuspensePlaceholder', () => {
         jest.advanceTimersByTime(1000);
 
         expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']);
-        expect(Scheduler).toFlushExpired(['Loaded']);
+
+        ReactNoop.flushSync();
+
+        expect(Scheduler).toHaveYielded(['Loaded']);
         expect(ReactNoop).toMatchRenderedOutput('LoadedNew');
         expect(onRender).toHaveBeenCalledTimes(4);
 

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
@@ -1088,9 +1088,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       expect(Scheduler).toHaveYielded(['Suspend! [Result]', 'Loading...']);
       expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);
 
-      await resolveText('Result');
+      await ReactNoop.act(async () => {
+        resolveText('Result');
+      });
 
-      expect(Scheduler).toFlushExpired(['Result']);
+      expect(Scheduler).toHaveYielded(['Result']);
       expect(ReactNoop.getChildren()).toEqual([span('Result')]);
     });
 
@@ -1156,8 +1158,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
         </>,
       );
 
-      await resolveText('Step: 2');
-      expect(Scheduler).toFlushExpired(['Step: 2']);
+      await ReactNoop.act(async () => {
+        resolveText('Step: 2');
+      });
+      expect(Scheduler).toHaveYielded(['Step: 2']);
       expect(ReactNoop).toMatchRenderedOutput(
         <>
           <span prop="Step: 2" />
@@ -1227,9 +1231,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
         </>,
       );
 
-      await resolveText('B');
+      await ReactNoop.act(async () => {
+        resolveText('B');
+      });
 
-      expect(Scheduler).toFlushExpired(['B']);
+      expect(Scheduler).toHaveYielded(['B']);
       expect(ReactNoop).toMatchRenderedOutput(
         <>
           <span prop="A" />
@@ -1271,9 +1277,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       ]);
       expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);
 
-      await resolveText('Hi');
+      await ReactNoop.act(async () => {
+        resolveText('Hi');
+      });
 
-      expect(Scheduler).toFlushExpired([
+      expect(Scheduler).toHaveYielded([
         'constructor',
         'Hi',
         'componentDidMount',
@@ -1316,8 +1324,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
         'Loading...',
       ]);
       expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);
-      await resolveText('Hi');
-      expect(Scheduler).toFlushExpired(['Hi']);
+      await ReactNoop.act(async () => {
+        resolveText('Hi');
+      });
+      expect(Scheduler).toHaveYielded(['Hi']);
       expect(ReactNoop.getChildren()).toEqual([span('Hi')]);
     });
 
@@ -1360,8 +1370,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
           </>,
         ]);
 
-        await resolveText('Hi');
-        expect(Scheduler).toFlushExpired(['Hi']);
+        await ReactNoop.act(async () => {
+          resolveText('Hi');
+        });
+        expect(Scheduler).toHaveYielded(['Hi']);
       });
     } else {
       // @gate enableCache
@@ -1401,9 +1413,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
           'Child is hidden: true',
         ]);
 
-        await resolveText('Hi');
+        await ReactNoop.act(async () => {
+          resolveText('Hi');
+        });
 
-        expect(Scheduler).toFlushExpired(['Hi']);
+        expect(Scheduler).toHaveYielded(['Hi']);
       });
     }
 
@@ -1647,9 +1661,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       </>,
     );
 
-    await resolveText('B');
+    await ReactNoop.act(async () => {
+      resolveText('B');
+    });
 
-    expect(Scheduler).toFlushAndYield([
+    expect(Scheduler).toHaveYielded([
       'B',
       'Destroy Layout Effect [Loading...]',
       'Layout Effect [B]',
@@ -1681,9 +1697,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       'Effect [Loading...]',
     ]);
 
-    await resolveText('B2');
+    await ReactNoop.act(async () => {
+      resolveText('B2');
+    });
 
-    expect(Scheduler).toFlushAndYield([
+    expect(Scheduler).toHaveYielded([
       'B2',
       'Destroy Layout Effect [Loading...]',
       'Destroy Layout Effect [B]',