Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Land enableSyncMicrotasks
Browse files Browse the repository at this point in the history
acdlite committed Mar 19, 2021

Verified

This commit was signed with the committer’s verified signature.
nalimilan Milan Bouchet-Valat
1 parent f6bc9c8 commit b729510
Showing 18 changed files with 100 additions and 60 deletions.
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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');
});
});
});
Original file line number Diff line number Diff line change
@@ -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 {
Original file line number Diff line number Diff line change
@@ -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 {
35 changes: 25 additions & 10 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.js
Original file line number Diff line number Diff line change
@@ -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');
});

Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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 () => {
Original file line number Diff line number Diff line change
@@ -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([
2 changes: 0 additions & 2 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
@@ -150,6 +150,4 @@ export const enableRecursiveCommitTraversal = false;

export const disableSchedulerTimeoutInWorkLoop = false;

export const enableSyncMicroTasks = false;

export const enableLazyContextPropagation = false;
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
@@ -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.
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
@@ -56,7 +56,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.
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
@@ -56,7 +56,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.
Original file line number Diff line number Diff line change
@@ -56,7 +56,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.
Original file line number Diff line number Diff line change
@@ -56,7 +56,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.
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
@@ -56,7 +56,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.
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
@@ -56,7 +56,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.
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
@@ -55,5 +55,4 @@ export const enableUseRefAccessWarning = __VARIANT__;

export const enableProfilerNestedUpdateScheduledHook = __VARIANT__;
export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;
export const enableSyncMicroTasks = __VARIANT__;
export const enableLazyContextPropagation = __VARIANT__;
1 change: 0 additions & 1 deletion packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
@@ -31,7 +31,6 @@ export const {
enableUseRefAccessWarning,
disableNativeComponentFrames,
disableSchedulerTimeoutInWorkLoop,
enableSyncMicroTasks,
enableLazyContextPropagation,
} = dynamicFeatureFlags;

0 comments on commit b729510

Please sign in to comment.