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 1 commit
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
getCurrentUpdateLanePriority,
setCurrentUpdateLanePriority,
} from './ReactFiberLane.new';
import {scheduleMicrotask, supportsMicrotasks} from './ReactFiberHostConfig';

const {
unstable_runWithPriority: Scheduler_runWithPriority,
Expand Down Expand Up @@ -147,10 +148,14 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
// 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 (supportsMicrotasks) {
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved
scheduleMicrotask(flushSyncCallbackQueueImpl);
} else {
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
getCurrentUpdateLanePriority,
setCurrentUpdateLanePriority,
} from './ReactFiberLane.old';
import {scheduleMicrotask, supportsMicrotasks} from './ReactFiberHostConfig';

const {
unstable_runWithPriority: Scheduler_runWithPriority,
Expand Down Expand Up @@ -147,10 +148,14 @@ export function scheduleSyncCallback(callback: SchedulerCallback) {
// 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 (supportsMicrotasks) {
scheduleMicrotask(flushSyncCallbackQueueImpl);
} else {
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,11 +1807,21 @@ 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)']);
expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);

if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
// Flush microtasks.
await null;

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

// effects get forced on exiting act()
Expand Down
12 changes: 10 additions & 2 deletions packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,16 @@ describe('ReactOffscreen', () => {
<Text text="Outside" />
</>,
);
// Should not defer the hidden tree
expect(Scheduler).toFlushUntilNextPaint(['A', 'Outside']);
if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
// Flush microtasks.
await null;
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved

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

// Because the render expired, React should finish the tree without
// consulting `shouldYield` again
expect(Scheduler).toFlushExpired(['B', 'C']);
if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
await null;

// Because the render expired, React should finish the tree without
// consulting `shouldYield` again
expect(Scheduler).toHaveYielded(['B', 'C']);
} else {
// Because the render expired, React should finish the tree without
// consulting `shouldYield` again
expect(Scheduler).toFlushExpired(['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,14 @@ describe('ReactSuspensePlaceholder', () => {
jest.advanceTimersByTime(1000);

expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']);
expect(Scheduler).toFlushExpired(['Loaded']);
if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
// Flush microtasks
await null;

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

Expand Down Expand Up @@ -378,7 +385,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 +434,15 @@ describe('ReactSuspensePlaceholder', () => {
jest.advanceTimersByTime(1000);

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

if (gate(flags => flags.enableDiscreteEventMicroTasks)) {
// Flush microtasks
await null;

expect(Scheduler).toHaveYielded(['Loaded']);
} else {
expect(Scheduler).toFlushExpired(['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
Original file line number Diff line number Diff line change
Expand Up @@ -1794,7 +1794,7 @@ describe('useMutableSource', () => {
});

// @gate experimental
it('should not misidentify mutations after render as side effects', () => {
it('should not misidentify mutations after render as side effects', async () => {
const source = createSource('initial');
const mutableSource = createMutableSource(
source,
Expand All @@ -1811,15 +1811,16 @@ describe('useMutableSource', () => {
return null;
}

act(() => {
await act(async () => {
ReactNoop.renderLegacySyncRoot(
<React.StrictMode>
<MutateDuringRead />
</React.StrictMode>,
);
expect(Scheduler).toFlushAndYieldThrough([
'MutateDuringRead:initial',
]);
});
expect(Scheduler).toHaveYielded(['MutateDuringRead:initial']);

await act(async () => {
source.value = 'updated';
});
expect(Scheduler).toHaveYielded(['MutateDuringRead:updated']);
Expand Down