Skip to content

Commit

Permalink
Rendering tasks should not jump the queue (#16284)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored Aug 3, 2019
1 parent c4c9f08 commit 6b565ce
Show file tree
Hide file tree
Showing 9 changed files with 109 additions and 1 deletion.
6 changes: 5 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
revertPassiveEffectsChange,
warnAboutUnmockedScheduler,
flushSuspenseFallbacksInTests,
disableSchedulerTimeoutBasedOnReactExpirationTime,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -530,7 +531,10 @@ function scheduleCallbackForRoot(
);
} else {
let options = null;
if (expirationTime !== Never) {
if (
!disableSchedulerTimeoutBasedOnReactExpirationTime &&
expirationTime !== Never
) {
let timeout = expirationTimeToMs(expirationTime) - now();
options = {timeout};
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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');
});
});
2 changes: 2 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,5 @@ export const enableSuspenseCallback = false;
export const warnAboutDefaultPropsOnFunctionComponents = false;

export const disableLegacyContext = false;

export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.persistent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export const {
revertPassiveEffectsChange,
enableUserBlockingEvents,
disableLegacyContext,
disableSchedulerTimeoutBasedOnReactExpirationTime,
} = require('ReactFeatureFlags');

// In www, we have experimental support for gathering data
Expand Down

0 comments on commit 6b565ce

Please sign in to comment.