Skip to content

Commit

Permalink
Schedule sync updates in microtask (#20872)
Browse files Browse the repository at this point in the history
* Schedule sync updates in microtask

* Updates from review

* Fix comment
  • Loading branch information
rickhanlonii authored Feb 25, 2021
1 parent 90bde65 commit c581cdd
Show file tree
Hide file tree
Showing 23 changed files with 121 additions and 51 deletions.
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

0 comments on commit c581cdd

Please sign in to comment.