Land enableSyncMicrotasks (#20979)

Author: acdlite

packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js

@@ -243,11 +243,12 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
       mouseOverEvent.initEvent('mouseover', true, true);
       dispatchAndSetCurrentEvent(target.current, mouseOverEvent);
 
-      // 3s should be enough to expire the updates
-      Scheduler.unstable_advanceTime(3000);
-      expect(Scheduler).toFlushExpired([]);
-      expect(container.textContent).toEqual('hovered');
+      // Flush discrete updates
+      ReactDOM.flushSync();
+      // Since mouse over is not discrete, should not have updated yet
+      expect(container.textContent).toEqual('not hovered');
     });
+    expect(container.textContent).toEqual('hovered');
   });
 
   // @gate experimental
@@ -275,11 +276,12 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
       mouseEnterEvent.initEvent('mouseenter', true, true);
       dispatchAndSetCurrentEvent(target.current, mouseEnterEvent);
 
-      // 3s should be enough to expire the updates
-      Scheduler.unstable_advanceTime(3000);
-      expect(Scheduler).toFlushExpired([]);
-      expect(container.textContent).toEqual('hovered');
+      // Flush discrete updates
+      ReactDOM.flushSync();
+      // Since mouse end is not discrete, should not have updated yet
+      expect(container.textContent).toEqual('not hovered');
     });
+    expect(container.textContent).toEqual('hovered');
   });
 
   // @gate experimental

packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js

@@ -761,11 +761,12 @@ describe('ChangeEventPlugin', () => {
         mouseOverEvent.initEvent('mouseover', true, true);
         target.current.dispatchEvent(mouseOverEvent);
 
-        // 3s should be enough to expire the updates
-        Scheduler.unstable_advanceTime(3000);
-        expect(Scheduler).toFlushExpired([]);
-        expect(container.textContent).toEqual('hovered');
+        // Flush discrete updates
+        ReactDOM.flushSync();
+        // Since mouse enter/leave is not discrete, should not have updated yet
+        expect(container.textContent).toEqual('not hovered');
       });
+      expect(container.textContent).toEqual('hovered');
     });
   });
 });

packages/react-reconciler/src/SchedulerWithReactIntegration.new.js

@@ -13,10 +13,7 @@ import type {ReactPriorityLevel} from './ReactInternalTypes';
 // CommonJS interop named imports.
 import * as Scheduler from 'scheduler';
 import {__interactionsRef} from 'scheduler/tracing';
-import {
-  enableSchedulerTracing,
-  enableSyncMicroTasks,
-} from 'shared/ReactFeatureFlags';
+import {enableSchedulerTracing} from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import {
   SyncLanePriority,
@@ -139,7 +136,7 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
 
     // TODO: Figure out how to remove this It's only here as a last resort if we
     // forget to explicitly flush.
-    if (enableSyncMicroTasks && supportsMicrotasks) {
+    if (supportsMicrotasks) {
       // Flush the queue in a microtask.
       scheduleMicrotask(flushSyncCallbackQueueImpl);
     } else {

packages/react-reconciler/src/SchedulerWithReactIntegration.old.js

@@ -13,10 +13,7 @@ import type {ReactPriorityLevel} from './ReactInternalTypes';
 // CommonJS interop named imports.
 import * as Scheduler from 'scheduler';
 import {__interactionsRef} from 'scheduler/tracing';
-import {
-  enableSchedulerTracing,
-  enableSyncMicroTasks,
-} from 'shared/ReactFeatureFlags';
+import {enableSchedulerTracing} from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import {
   SyncLanePriority,
@@ -139,7 +136,7 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
 
     // TODO: Figure out how to remove this It's only here as a last resort if we
     // forget to explicitly flush.
-    if (enableSyncMicroTasks && supportsMicrotasks) {
+    if (supportsMicrotasks) {
       // Flush the queue in a microtask.
       scheduleMicrotask(flushSyncCallbackQueueImpl);
     } else {

packages/react-reconciler/src/__tests__/ReactExpiration-test.js

@@ -103,23 +103,32 @@ describe('ReactExpiration', () => {
     return {type: 'span', children: [], prop, hidden: false};
   }
 
+  function flushNextRenderIfExpired() {
+    // This will start rendering the next level of work. If the work hasn't
+    // expired yet, React will exit without doing anything. If it has expired,
+    // it will schedule a sync task.
+    Scheduler.unstable_flushExpired();
+    // Flush the sync task.
+    ReactNoop.flushSync();
+  }
+
   it('increases priority of updates as time progresses', () => {
     ReactNoop.render(<span prop="done" />);
 
     expect(ReactNoop.getChildren()).toEqual([]);
 
     // Nothing has expired yet because time hasn't advanced.
-    ReactNoop.flushExpired();
+    flushNextRenderIfExpired();
     expect(ReactNoop.getChildren()).toEqual([]);
 
     // Advance time a bit, but not enough to expire the low pri update.
     ReactNoop.expire(4500);
-    ReactNoop.flushExpired();
+    flushNextRenderIfExpired();
     expect(ReactNoop.getChildren()).toEqual([]);
 
     // Advance by another second. Now the update should expire and flush.
-    ReactNoop.expire(1000);
-    ReactNoop.flushExpired();
+    ReactNoop.expire(500);
+    flushNextRenderIfExpired();
     expect(ReactNoop.getChildren()).toEqual([span('done')]);
   });
 
@@ -323,7 +332,8 @@ describe('ReactExpiration', () => {
 
     Scheduler.unstable_advanceTime(10000);
 
-    expect(Scheduler).toFlushExpired(['D', 'E']);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded(['D', 'E']);
     expect(root).toMatchRenderedOutput('ABCDE');
   });
 
@@ -351,7 +361,8 @@ describe('ReactExpiration', () => {
 
     Scheduler.unstable_advanceTime(10000);
 
-    expect(Scheduler).toFlushExpired(['D', 'E']);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded(['D', 'E']);
     expect(root).toMatchRenderedOutput('ABCDE');
   });
 
@@ -373,12 +384,14 @@ describe('ReactExpiration', () => {
     ReactNoop.render('Hi');
 
     // The update should not have expired yet.
-    expect(Scheduler).toFlushExpired([]);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded([]);
     expect(ReactNoop).toMatchRenderedOutput(null);
 
     // Advance the time some more to expire the update.
     Scheduler.unstable_advanceTime(10000);
-    expect(Scheduler).toFlushExpired([]);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded([]);
     expect(ReactNoop).toMatchRenderedOutput('Hi');
   });
 
@@ -391,15 +404,17 @@ describe('ReactExpiration', () => {
     Scheduler.unstable_advanceTime(10000);
 
     ReactNoop.render('Hi');
-    expect(Scheduler).toFlushExpired([]);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded([]);
     expect(ReactNoop).toMatchRenderedOutput(null);
 
     // 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');
-    expect(Scheduler).toFlushExpired([]);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded([]);
     expect(ReactNoop).toMatchRenderedOutput('Hi');
   });
 

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js

@@ -402,7 +402,7 @@ describe('ReactIncrementalErrorHandling', () => {
     expect(ReactNoop.getChildren()).toEqual([]);
   });
 
-  it('retries one more time if an error occurs during a render that expires midway through the tree', () => {
+  it('retries one more time if an error occurs during a render that expires midway through the tree', async () => {
     function Oops({unused}) {
       Scheduler.unstable_yieldValue('Oops');
       throw new Error('Oops');
@@ -432,7 +432,11 @@ describe('ReactIncrementalErrorHandling', () => {
 
     // Expire the render midway through
     Scheduler.unstable_advanceTime(10000);
-    expect(() => Scheduler.unstable_flushExpired()).toThrow('Oops');
+
+    expect(() => {
+      Scheduler.unstable_flushExpired();
+      ReactNoop.flushSync();
+    }).toThrow('Oops');
 
     expect(Scheduler).toHaveYielded([
       // The render expired, but we shouldn't throw out the partial work.

packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.js

@@ -32,6 +32,15 @@ describe('ReactIncrementalUpdates', () => {
     return {type: 'span', children: [], prop, hidden: false};
   }
 
+  function flushNextRenderIfExpired() {
+    // This will start rendering the next level of work. If the work hasn't
+    // expired yet, React will exit without doing anything. If it has expired,
+    // it will schedule a sync task.
+    Scheduler.unstable_flushExpired();
+    // Flush the sync task.
+    ReactNoop.flushSync();
+  }
+
   it('applies updates in order of priority', () => {
     let state;
     class Foo extends React.Component {
@@ -469,7 +478,8 @@ describe('ReactIncrementalUpdates', () => {
 
     ReactNoop.act(() => {
       ReactNoop.render(<App />);
-      expect(Scheduler).toFlushExpired([]);
+      flushNextRenderIfExpired();
+      expect(Scheduler).toHaveYielded([]);
       expect(Scheduler).toFlushAndYield([
         'Render: 0',
         'Commit: 0',
@@ -479,7 +489,8 @@ describe('ReactIncrementalUpdates', () => {
       Scheduler.unstable_advanceTime(10000);
 
       setCount(2);
-      expect(Scheduler).toFlushExpired([]);
+      flushNextRenderIfExpired();
+      expect(Scheduler).toHaveYielded([]);
     });
   });
 
@@ -497,7 +508,8 @@ describe('ReactIncrementalUpdates', () => {
     Scheduler.unstable_advanceTime(10000);
 
     ReactNoop.render(<Text text="B" />);
-    expect(Scheduler).toFlushExpired([]);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded([]);
   });
 
   it('regression: does not expire soon due to previous expired work', () => {
@@ -508,12 +520,14 @@ describe('ReactIncrementalUpdates', () => {
 
     ReactNoop.render(<Text text="A" />);
     Scheduler.unstable_advanceTime(10000);
-    expect(Scheduler).toFlushExpired(['A']);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded(['A']);
 
     Scheduler.unstable_advanceTime(10000);
 
     ReactNoop.render(<Text text="B" />);
-    expect(Scheduler).toFlushExpired([]);
+    flushNextRenderIfExpired();
+    expect(Scheduler).toHaveYielded([]);
   });
 
   it('when rebasing, does not exclude updates that were already committed, regardless of priority', async () => {

packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js

@@ -984,6 +984,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     expect(ReactNoop.getChildren()).toEqual([span('C')]);
   });
 
+  // TODO: This test was written against the old Expiration Times
+  // implementation. It doesn't really test what it was intended to test
+  // anymore, because all updates to the same queue get entangled together.
+  // Even if they haven't expired. Consider either deleting or rewriting.
   // @gate enableCache
   it('flushes all expired updates in a single batch', async () => {
     class Foo extends React.Component {
@@ -1013,10 +1017,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     jest.advanceTimersByTime(1000);
     ReactNoop.render(<Foo text="goodbye" />);
 
-    Scheduler.unstable_advanceTime(10000);
-    jest.advanceTimersByTime(10000);
-
-    expect(Scheduler).toFlushExpired([
+    expect(Scheduler).toFlushAndYield([
       'Suspend! [goodbye]',
       'Loading...',
       'Commit: goodbye',
@@ -1797,12 +1798,32 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     await advanceTimers(5000);
 
     // Retry with the new content.
-    expect(Scheduler).toFlushAndYield([
-      'A',
-      // B still suspends
-      'Suspend! [B]',
-      'Loading more...',
-    ]);
+    if (gate(flags => flags.disableSchedulerTimeoutInWorkLoop)) {
+      expect(Scheduler).toFlushAndYield([
+        'A',
+        // B still suspends
+        'Suspend! [B]',
+        'Loading more...',
+      ]);
+    } else {
+      // In this branch, right as we start rendering, we detect that the work
+      // has expired (via Scheduler's didTimeout argument) and re-schedule the
+      // work as synchronous. Since sync work does not flow through Scheduler,
+      // we need to use `flushSync`.
+      //
+      // Usually we would use `act`, which fluses both sync work and Scheduler
+      // work, but that would also force the fallback to display, and this test
+      // is specifically about whether we delay or show the fallback.
+      expect(Scheduler).toFlushAndYield([]);
+      // This will flush the synchronous callback we just scheduled.
+      ReactNoop.flushSync();
+      expect(Scheduler).toHaveYielded([
+        'A',
+        // B still suspends
+        'Suspend! [B]',
+        'Loading more...',
+      ]);
+    }
     // Because we've already been waiting for so long we've exceeded
     // our threshold and we show the next level immediately.
     expect(ReactNoop.getChildren()).toEqual([

packages/shared/ReactFeatureFlags.js

@@ -150,6 +150,4 @@ export const enableRecursiveCommitTraversal = false;
 
 export const disableSchedulerTimeoutInWorkLoop = false;
 
-export const enableSyncMicroTasks = false;
-
 export const enableLazyContextPropagation = false;

packages/shared/forks/ReactFeatureFlags.native-fb.js

@@ -57,7 +57,6 @@ export const enableUseRefAccessWarning = false;
 
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
-export const enableSyncMicroTasks = false;
 export const enableLazyContextPropagation = false;
 
 // Flow magic to verify the exports of this file match the original version.