From 6491199dcb365b733eab5cae87ded630d1b60089 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 2 Aug 2019 17:26:08 -0700 Subject: [PATCH] Rendering tasks should not jump the queue 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. --- .../src/ReactFiberWorkLoop.js | 6 +- ...asedOnReactExpirationTime-test.internal.js | 96 +++++++++++++++++++ packages/shared/ReactFeatureFlags.js | 2 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.persistent.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 9 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactDisableSchedulerTimeoutBasedOnReactExpirationTime-test.internal.js diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 76698b67185..edca5c6458c 100644 --- 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 new file mode 100644 index 00000000000..83603d625fb --- /dev/null +++ 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 ; + }; + 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 ( + }> + {show ? : null} + {show ? : null} + + ); + } + + const root = ReactNoop.createRoot(null); + + root.render(); + expect(Scheduler).toFlushAndYield([]); + + root.render(); + 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 index da01edb1387..842147b03af 100644 --- 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 index 6b95b92d207..894d57f02e6 100644 --- 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 index 08525d0c093..387bd8656a8 100644 --- 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 index 478586ff80a..8aec846c197 100644 --- 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 index 91bfab22efa..b1f941bec09 100644 --- 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 index 28f7d534292..0a9df80e9a8 100644 --- 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 index 3f2c4c6b519..4999747c44e 100644 --- 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