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

Schedule sync updates in microtask #20872

Merged
merged 3 commits into from
Feb 25, 2021
Merged
Show file tree
Hide file tree
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
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,7 @@ describe('ReactDOMServerPartialHydration', () => {
resolve();
await promise;
Scheduler.unstable_flushAll();
await null;
jest.runAllTimers();

// We should now have hydrated with a ref on the existing span.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,7 @@ describe('ReactDOMServerHydration', () => {
jest.runAllTimers();
await Promise.resolve();
Scheduler.unstable_flushAll();
await null;
expect(element.textContent).toBe('Hello world');
});

Expand Down
18 changes: 13 additions & 5 deletions packages/react-reconciler/src/SchedulerWithReactIntegration.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ import {__interactionsRef} from 'scheduler/tracing';
import {
enableSchedulerTracing,
decoupleUpdatePriorityFromScheduler,
enableSyncMicroTasks,
} from 'shared/ReactFeatureFlags';
import invariant from 'shared/invariant';
import {
SyncLanePriority,
getCurrentUpdateLanePriority,
setCurrentUpdateLanePriority,
} from './ReactFiberLane.new';
import {scheduleMicrotask, supportsMicrotasks} from './ReactFiberHostConfig';

const {
unstable_runWithPriority: Scheduler_runWithPriority,
Expand Down Expand Up @@ -144,13 +146,19 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
// the next tick, or earlier if something calls `flushSyncCallbackQueue`.
if (syncQueue === null) {
syncQueue = [callback];
// Flush the queue in the next tick, at the earliest.

// TODO: Figure out how to remove this It's only here as a last resort if we
// forget to explicitly flush.
immediateQueueCallbackNode = Scheduler_scheduleCallback(
Scheduler_ImmediatePriority,
flushSyncCallbackQueueImpl,
);
if (enableSyncMicroTasks && supportsMicrotasks) {
// Flush the queue in a microtask.
scheduleMicrotask(flushSyncCallbackQueueImpl);
} else {
// Flush the queue in the next tick.
immediateQueueCallbackNode = Scheduler_scheduleCallback(
Scheduler_ImmediatePriority,
flushSyncCallbackQueueImpl,
);
}
} else {
// Push onto existing queue. Don't need to schedule a callback because
// we already scheduled one when we created the queue.
Expand Down
18 changes: 13 additions & 5 deletions packages/react-reconciler/src/SchedulerWithReactIntegration.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ import {__interactionsRef} from 'scheduler/tracing';
import {
enableSchedulerTracing,
decoupleUpdatePriorityFromScheduler,
enableSyncMicroTasks,
} from 'shared/ReactFeatureFlags';
import invariant from 'shared/invariant';
import {
SyncLanePriority,
getCurrentUpdateLanePriority,
setCurrentUpdateLanePriority,
} from './ReactFiberLane.old';
import {scheduleMicrotask, supportsMicrotasks} from './ReactFiberHostConfig';

const {
unstable_runWithPriority: Scheduler_runWithPriority,
Expand Down Expand Up @@ -144,13 +146,19 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
// the next tick, or earlier if something calls `flushSyncCallbackQueue`.
if (syncQueue === null) {
syncQueue = [callback];
// Flush the queue in the next tick, at the earliest.

// TODO: Figure out how to remove this It's only here as a last resort if we
// forget to explicitly flush.
immediateQueueCallbackNode = Scheduler_scheduleCallback(
Scheduler_ImmediatePriority,
flushSyncCallbackQueueImpl,
);
if (enableSyncMicroTasks && supportsMicrotasks) {
// Flush the queue in a microtask.
scheduleMicrotask(flushSyncCallbackQueueImpl);
} else {
// Flush the queue in the next tick.
immediateQueueCallbackNode = Scheduler_scheduleCallback(
Scheduler_ImmediatePriority,
flushSyncCallbackQueueImpl,
);
}
} else {
// Push onto existing queue. Don't need to schedule a callback because
// we already scheduled one when we created the queue.
Expand Down
11 changes: 6 additions & 5 deletions packages/react-reconciler/src/__tests__/ReactExpiration-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -598,12 +598,13 @@ describe('ReactExpiration', () => {
// second one.
Scheduler.unstable_advanceTime(1000);
// Attempt to interrupt with a high pri update.
updateHighPri();
await ReactNoop.act(async () => {
updateHighPri();
});

// The first update expired, so first will finish it without
// interrupting. But not the second update, which hasn't expired yet.
expect(Scheduler).toFlushExpired(['Sibling']);
expect(Scheduler).toFlushAndYield([
expect(Scheduler).toHaveYielded([
// The first update expired
'Sibling',
// Then render the high pri update
'High pri: 1',
'Normal pri: 1',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1792,7 +1792,7 @@ describe('ReactHooksWithNoopRenderer', () => {
it(
'in legacy mode, useEffect is deferred and updates finish synchronously ' +
'(in a single batch)',
() => {
async () => {
function Counter(props) {
const [count, updateCount] = useState('(empty)');
useEffect(() => {
Expand All @@ -1807,10 +1807,12 @@ describe('ReactHooksWithNoopRenderer', () => {
}, [props.count]);
return <Text text={'Count: ' + count} />;
}
act(() => {
await act(async () => {
ReactNoop.renderLegacySyncRoot(<Counter count={0} />);

// Even in legacy mode, effects are deferred until after paint
expect(Scheduler).toFlushAndYieldThrough(['Count: (empty)']);
ReactNoop.flushSync();
expect(Scheduler).toHaveYielded(['Count: (empty)']);
expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1400,6 +1400,10 @@ describe('ReactIncrementalErrorHandling', () => {
'BrokenRenderAndUnmount componentWillUnmount',
]);
expect(ReactNoop.getChildren()).toEqual([]);

expect(() => {
ReactNoop.flushSync();
}).toThrow('One does not simply unmount me.');
});

it('does not interrupt unmounting if detaching a ref throws', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,11 @@ describe('ReactOffscreen', () => {
<Text text="Outside" />
</>,
);

ReactNoop.flushSync();

// Should not defer the hidden tree
expect(Scheduler).toFlushUntilNextPaint(['A', 'Outside']);
expect(Scheduler).toHaveYielded(['A', 'Outside']);
});
expect(root).toMatchRenderedOutput(
<>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -569,9 +569,11 @@ describe(
ReactNoop.render(<App />);
});

ReactNoop.flushSync();

// Because the render expired, React should finish the tree without
// consulting `shouldYield` again
expect(Scheduler).toFlushExpired(['B', 'C']);
expect(Scheduler).toHaveYielded(['B', 'C']);
});
});
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,11 @@ describe('ReactSuspenseList', () => {
</>,
);

await C.resolve();
await ReactNoop.act(async () => {
C.resolve();
});

expect(Scheduler).toFlushAndYield(['C']);
expect(Scheduler).toHaveYielded(['C']);

expect(ReactNoop).toMatchRenderedOutput(
<>
Expand All @@ -304,9 +306,11 @@ describe('ReactSuspenseList', () => {
</>,
);

await B.resolve();
await ReactNoop.act(async () => {
B.resolve();
});

expect(Scheduler).toFlushAndYield(['B']);
expect(Scheduler).toHaveYielded(['B']);

expect(ReactNoop).toMatchRenderedOutput(
<>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ describe('ReactSuspensePlaceholder', () => {
});

describe('when suspending during mount', () => {
it('properly accounts for base durations when a suspended times out in a legacy tree', () => {
it('properly accounts for base durations when a suspended times out in a legacy tree', async () => {
ReactNoop.renderLegacySyncRoot(<App shouldSuspend={true} />);
expect(Scheduler).toHaveYielded([
'App',
Expand All @@ -331,7 +331,10 @@ describe('ReactSuspensePlaceholder', () => {
jest.advanceTimersByTime(1000);

expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']);
expect(Scheduler).toFlushExpired(['Loaded']);

ReactNoop.flushSync();

expect(Scheduler).toHaveYielded(['Loaded']);
expect(ReactNoop).toMatchRenderedOutput('LoadedText');
expect(onRender).toHaveBeenCalledTimes(2);

Expand Down Expand Up @@ -378,7 +381,7 @@ describe('ReactSuspensePlaceholder', () => {
});

describe('when suspending during update', () => {
it('properly accounts for base durations when a suspended times out in a legacy tree', () => {
it('properly accounts for base durations when a suspended times out in a legacy tree', async () => {
ReactNoop.renderLegacySyncRoot(
<App shouldSuspend={false} textRenderDuration={5} />,
);
Expand Down Expand Up @@ -427,7 +430,10 @@ describe('ReactSuspensePlaceholder', () => {
jest.advanceTimersByTime(1000);

expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']);
expect(Scheduler).toFlushExpired(['Loaded']);

ReactNoop.flushSync();

expect(Scheduler).toHaveYielded(['Loaded']);
expect(ReactNoop).toMatchRenderedOutput('LoadedNew');
expect(onRender).toHaveBeenCalledTimes(4);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1088,9 +1088,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
expect(Scheduler).toHaveYielded(['Suspend! [Result]', 'Loading...']);
expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);

await resolveText('Result');
await ReactNoop.act(async () => {
resolveText('Result');
});

expect(Scheduler).toFlushExpired(['Result']);
expect(Scheduler).toHaveYielded(['Result']);
expect(ReactNoop.getChildren()).toEqual([span('Result')]);
});

Expand Down Expand Up @@ -1156,8 +1158,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
</>,
);

await resolveText('Step: 2');
expect(Scheduler).toFlushExpired(['Step: 2']);
await ReactNoop.act(async () => {
resolveText('Step: 2');
});
expect(Scheduler).toHaveYielded(['Step: 2']);
expect(ReactNoop).toMatchRenderedOutput(
<>
<span prop="Step: 2" />
Expand Down Expand Up @@ -1227,9 +1231,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
</>,
);

await resolveText('B');
await ReactNoop.act(async () => {
resolveText('B');
});

expect(Scheduler).toFlushExpired(['B']);
expect(Scheduler).toHaveYielded(['B']);
expect(ReactNoop).toMatchRenderedOutput(
<>
<span prop="A" />
Expand Down Expand Up @@ -1271,9 +1277,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
]);
expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);

await resolveText('Hi');
await ReactNoop.act(async () => {
resolveText('Hi');
});

expect(Scheduler).toFlushExpired([
expect(Scheduler).toHaveYielded([
'constructor',
'Hi',
'componentDidMount',
Expand Down Expand Up @@ -1316,8 +1324,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
'Loading...',
]);
expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);
await resolveText('Hi');
expect(Scheduler).toFlushExpired(['Hi']);
await ReactNoop.act(async () => {
resolveText('Hi');
});
expect(Scheduler).toHaveYielded(['Hi']);
expect(ReactNoop.getChildren()).toEqual([span('Hi')]);
});

Expand Down Expand Up @@ -1360,8 +1370,10 @@ describe('ReactSuspenseWithNoopRenderer', () => {
</>,
]);

await resolveText('Hi');
expect(Scheduler).toFlushExpired(['Hi']);
await ReactNoop.act(async () => {
resolveText('Hi');
});
expect(Scheduler).toHaveYielded(['Hi']);
});
} else {
// @gate enableCache
Expand Down Expand Up @@ -1401,9 +1413,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
'Child is hidden: true',
]);

await resolveText('Hi');
await ReactNoop.act(async () => {
resolveText('Hi');
});

expect(Scheduler).toFlushExpired(['Hi']);
expect(Scheduler).toHaveYielded(['Hi']);
});
}

Expand Down Expand Up @@ -1647,9 +1661,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
</>,
);

await resolveText('B');
await ReactNoop.act(async () => {
resolveText('B');
});

expect(Scheduler).toFlushAndYield([
expect(Scheduler).toHaveYielded([
'B',
'Destroy Layout Effect [Loading...]',
'Layout Effect [B]',
Expand Down Expand Up @@ -1681,9 +1697,11 @@ describe('ReactSuspenseWithNoopRenderer', () => {
'Effect [Loading...]',
]);

await resolveText('B2');
await ReactNoop.act(async () => {
resolveText('B2');
});

expect(Scheduler).toFlushAndYield([
expect(Scheduler).toHaveYielded([
'B2',
'Destroy Layout Effect [Loading...]',
'Destroy Layout Effect [B]',
Expand Down
Loading