Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Land enableSyncMicrotasks #20979

Merged
merged 1 commit into from
Mar 19, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Land enableSyncMicrotasks
acdlite committed Mar 19, 2021
commit b729510e0d58eefb40f1893db7627b3f3e810673
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;