Rendering tasks should not jump the queue (#16284)

acdlite · web-flow · commit 6b565ce73665 · 2019-08-02T17:52:32.000-07:00
When React schedules a rendering task, it passes a `timeout` option
based on its expiration time. This is intended to avoid starvation
by other React updates. However, it also affects the relative priority
of React tasks and other Scheduler tasks at the same level, like
data processing.

This adds a feature flag to disable passing a `timeout` option to
Scheduler. React tasks will always append themselves to the end of
the queue, without jumping ahead of already scheduled tasks.

This does not affect the order in which React updates within a single
root are processed, but it could affect updates across multiple roots.

This also doesn't remove the expiration from Scheduler. It only means
that React tasks are not given special treatment.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -27,6 +27,7 @@ import {
   revertPassiveEffectsChange,
   warnAboutUnmockedScheduler,
   flushSuspenseFallbacksInTests,
+  disableSchedulerTimeoutBasedOnReactExpirationTime,
 } from 'shared/ReactFeatureFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import invariant from 'shared/invariant';
@@ -530,7 +531,10 @@ function scheduleCallbackForRoot(
       );
     } else {
       let options = null;
-      if (expirationTime !== Never) {
+      if (
+        !disableSchedulerTimeoutBasedOnReactExpirationTime &&
+        expirationTime !== Never
+      ) {
         let timeout = expirationTimeToMs(expirationTime) - now();
         options = {timeout};
       }
diff --git a/packages/react-reconciler/src/__tests__/ReactDisableSchedulerTimeoutBasedOnReactExpirationTime-test.internal.js b/packages/react-reconciler/src/__tests__/ReactDisableSchedulerTimeoutBasedOnReactExpirationTime-test.internal.js
@@ -0,0 +1,96 @@
+let React;
+let ReactFeatureFlags;
+let ReactNoop;
+let Scheduler;
+let Suspense;
+let scheduleCallback;
+let NormalPriority;
+
+describe('ReactSuspenseList', () => {
+  beforeEach(() => {
+    jest.resetModules();
+    ReactFeatureFlags = require('shared/ReactFeatureFlags');
+    ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
+    ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
+    ReactFeatureFlags.disableSchedulerTimeoutBasedOnReactExpirationTime = true;
+    React = require('react');
+    ReactNoop = require('react-noop-renderer');
+    Scheduler = require('scheduler');
+    Suspense = React.Suspense;
+
+    scheduleCallback = Scheduler.unstable_scheduleCallback;
+    NormalPriority = Scheduler.unstable_NormalPriority;
+  });
+
+  function Text(props) {
+    Scheduler.unstable_yieldValue(props.text);
+    return props.text;
+  }
+
+  function createAsyncText(text) {
+    let resolved = false;
+    let Component = function() {
+      if (!resolved) {
+        Scheduler.unstable_yieldValue('Suspend! [' + text + ']');
+        throw promise;
+      }
+      return <Text text={text} />;
+    };
+    let promise = new Promise(resolve => {
+      Component.resolve = function() {
+        resolved = true;
+        return resolve();
+      };
+    });
+    return Component;
+  }
+
+  it('appends rendering tasks to the end of the priority queue', async () => {
+    const A = createAsyncText('A');
+    const B = createAsyncText('B');
+
+    function App({show}) {
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          {show ? <A /> : null}
+          {show ? <B /> : null}
+        </Suspense>
+      );
+    }
+
+    const root = ReactNoop.createRoot(null);
+
+    root.render(<App show={false} />);
+    expect(Scheduler).toFlushAndYield([]);
+
+    root.render(<App show={true} />);
+    expect(Scheduler).toFlushAndYield([
+      'Suspend! [A]',
+      'Suspend! [B]',
+      'Loading...',
+    ]);
+    expect(root).toMatchRenderedOutput(null);
+
+    Scheduler.unstable_advanceTime(2000);
+    expect(root).toMatchRenderedOutput(null);
+
+    scheduleCallback(NormalPriority, () => {
+      Scheduler.unstable_yieldValue('Resolve A');
+      A.resolve();
+    });
+    scheduleCallback(NormalPriority, () => {
+      Scheduler.unstable_yieldValue('Resolve B');
+      B.resolve();
+    });
+
+    // This resolves A and schedules a task for React to retry.
+    await expect(Scheduler).toFlushAndYieldThrough(['Resolve A']);
+
+    // The next task that flushes should be the one that resolves B. The render
+    // task should not jump the queue ahead of B.
+    await expect(Scheduler).toFlushAndYieldThrough(['Resolve B']);
+
+    expect(Scheduler).toFlushAndYield(['A', 'B']);
+    expect(root).toMatchRenderedOutput('AB');
+  });
+});
diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -94,3 +94,5 @@ export const enableSuspenseCallback = false;
 export const warnAboutDefaultPropsOnFunctionComponents = false;
 
 export const disableLegacyContext = false;
+
+export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -41,6 +41,7 @@ export const enableUserBlockingEvents = false;
 export const enableSuspenseCallback = false;
 export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const disableLegacyContext = false;
+export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {
diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -36,6 +36,7 @@ export const enableUserBlockingEvents = false;
 export const enableSuspenseCallback = false;
 export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const disableLegacyContext = false;
+export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {
diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js
@@ -36,6 +36,7 @@ export const enableUserBlockingEvents = false;
 export const enableSuspenseCallback = false;
 export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const disableLegacyContext = false;
+export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -36,6 +36,7 @@ export const enableUserBlockingEvents = false;
 export const enableSuspenseCallback = false;
 export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const disableLegacyContext = false;
+export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -36,6 +36,7 @@ export const enableUserBlockingEvents = false;
 export const enableSuspenseCallback = true;
 export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const disableLegacyContext = false;
+export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
 
 // Only used in www builds.
 export function addUserTimingListener() {
diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -22,6 +22,7 @@ export const {
   revertPassiveEffectsChange,
   enableUserBlockingEvents,
   disableLegacyContext,
+  disableSchedulerTimeoutBasedOnReactExpirationTime,
 } = require('ReactFeatureFlags');
 
 // In www, we have experimental support for gathering data
