Updates from review

facebook · Feb 25, 2021 · 4cea1e0 · 4cea1e0
1 parent 4b63f61
commit 4cea1e0
Show file tree

Hide file tree

Showing 17 changed files with 46 additions and 55 deletions.
diff --git a/packages/react-reconciler/src/SchedulerWithReactIntegration.new.js b/packages/react-reconciler/src/SchedulerWithReactIntegration.new.js
@@ -16,6 +16,7 @@ import {__interactionsRef} from 'scheduler/tracing';
 import {
   enableSchedulerTracing,
   decoupleUpdatePriorityFromScheduler,
+  enableSyncMicroTasks,
 } from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import {
@@ -145,12 +146,13 @@ 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.
-    if (supportsMicrotasks) {
+    if (enableSyncMicroTasks && supportsMicrotasks) {
+      // Flush the queue in a microtask.
       scheduleMicrotask(flushSyncCallbackQueueImpl);
     } else {
+      // 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,

diff --git a/packages/react-reconciler/src/SchedulerWithReactIntegration.old.js b/packages/react-reconciler/src/SchedulerWithReactIntegration.old.js
@@ -16,6 +16,7 @@ import {__interactionsRef} from 'scheduler/tracing';
 import {
   enableSchedulerTracing,
   decoupleUpdatePriorityFromScheduler,
+  enableSyncMicroTasks,
 } from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import {
@@ -145,12 +146,13 @@ 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.
-    if (supportsMicrotasks) {
+    if (enableSyncMicroTasks && supportsMicrotasks) {
+      // Flush the queue in a microtask.
       scheduleMicrotask(flushSyncCallbackQueueImpl);
     } else {
+      // 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,

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -1810,18 +1810,10 @@ describe('ReactHooksWithNoopRenderer', () => {
         await act(async () => {
           ReactNoop.renderLegacySyncRoot(<Counter count={0} />);
 
-          if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
-            // Flush microtasks.
-            await null;
-
-            // Even in legacy mode, effects are deferred until after paint
-            expect(Scheduler).toHaveYielded(['Count: (empty)']);
-            expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);
-          } else {
-            // Even in legacy mode, effects are deferred until after paint
-            expect(Scheduler).toFlushAndYieldThrough(['Count: (empty)']);
-            expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);
-          }
+          // Even in legacy mode, effects are deferred until after paint
+          ReactNoop.flushSync();
+          expect(Scheduler).toHaveYielded(['Count: (empty)']);
+          expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);
         });
 
         // effects get forced on exiting act()

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,16 +98,11 @@ describe('ReactOffscreen', () => {
           <Text text="Outside" />
         </>,
       );
-      if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
-        // Flush microtasks.
-        await null;
 
-        // Should not defer the hidden tree
-        expect(Scheduler).toHaveYielded(['A', 'Outside']);
-      } else {
-        // Should not defer the hidden tree
-        expect(Scheduler).toFlushUntilNextPaint(['A', 'Outside']);
-      }
+      ReactNoop.flushSync();
+
+      // Should not defer the hidden tree
+      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,17 +569,11 @@ describe(
           ReactNoop.render(<App />);
         });
 
-        if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
-          await null;
-
-          // Because the render expired, React should finish the tree without
-          // consulting `shouldYield` again
-          expect(Scheduler).toHaveYielded(['B', 'C']);
-        } else {
-          // Because the render expired, React should finish the tree without
-          // consulting `shouldYield` again
-          expect(Scheduler).toFlushExpired(['B', 'C']);
-        }
+        ReactNoop.flushSync();
+
+        // Because the render expired, React should finish the tree without
+        // consulting `shouldYield` again
+        expect(Scheduler).toHaveYielded(['B', 'C']);
       });
     });
   },

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js
@@ -331,14 +331,10 @@ describe('ReactSuspensePlaceholder', () => {
         jest.advanceTimersByTime(1000);
 
         expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']);
-        if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
-          // Flush microtasks
-          await null;
-
-          expect(Scheduler).toHaveYielded(['Loaded']);
-        } else {
-          expect(Scheduler).toFlushExpired(['Loaded']);
-        }
+
+        ReactNoop.flushSync();
+
+        expect(Scheduler).toHaveYielded(['Loaded']);
         expect(ReactNoop).toMatchRenderedOutput('LoadedText');
         expect(onRender).toHaveBeenCalledTimes(2);
 
@@ -435,14 +431,9 @@ describe('ReactSuspensePlaceholder', () => {
 
         expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']);
 
-        if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
-          // Flush microtasks
-          await null;
+        ReactNoop.flushSync();
 
-          expect(Scheduler).toHaveYielded(['Loaded']);
-        } else {
-          expect(Scheduler).toFlushExpired(['Loaded']);
-        }
+        expect(Scheduler).toHaveYielded(['Loaded']);
         expect(ReactNoop).toMatchRenderedOutput('LoadedNew');
         expect(onRender).toHaveBeenCalledTimes(4);
 

diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -151,4 +151,6 @@ export const enableNonInterruptingNormalPri = false;
 
 export const enableDiscreteEventMicroTasks = false;
 
+export const enableSyncMicroTasks = false;
+
 export const enableNativeEventPriorityInference = false;
diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -59,6 +59,7 @@ export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
+export const enableSyncMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 
 // Flow magic to verify the exports of this file match the original version.

diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -58,6 +58,7 @@ export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
+export const enableSyncMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 
 // Flow magic to verify the exports of this file match the original version.

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -58,6 +58,7 @@ export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
+export const enableSyncMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 
 // Flow magic to verify the exports of this file match the original version.

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js
@@ -58,6 +58,7 @@ export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
+export const enableSyncMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 
 // Flow magic to verify the exports of this file match the original version.

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -58,6 +58,7 @@ export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
+export const enableSyncMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 
 // Flow magic to verify the exports of this file match the original version.

diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js
@@ -58,6 +58,7 @@ export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
+export const enableSyncMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 
 // Flow magic to verify the exports of this file match the original version.

diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js
@@ -58,6 +58,7 @@ export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
+export const enableSyncMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 
 // Flow magic to verify the exports of this file match the original version.

diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js
@@ -57,4 +57,5 @@ export const enableProfilerNestedUpdateScheduledHook = __VARIANT__;
 export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;
 export const enableNonInterruptingNormalPri = __VARIANT__;
 export const enableDiscreteEventMicroTasks = __VARIANT__;
+export const enableSyncMicroTasks = __VARIANT__;
 export const enableNativeEventPriorityInference = __VARIANT__;
diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -33,6 +33,7 @@ export const {
   disableSchedulerTimeoutInWorkLoop,
   enableNonInterruptingNormalPri,
   enableDiscreteEventMicroTasks,
+  enableSyncMicroTasks,
   enableNativeEventPriorityInference,
 } = dynamicFeatureFlags;