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.

Sign up for GitHub

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

Jump to bottom

Add scheduleMicrotaskCallback, use it to schedule flushSync for discrete update priority #20658

Closed
wants to merge 1 commit into from
+111 −99
Closed

Add scheduleMicrotaskCallback, use it to schedule flushSync for discrete update priority #20658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 11 additions & 6 deletions packages/react-dom/src/__tests__/ReactDOMHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
let React;
let ReactDOM;
let Scheduler;
let act;

describe('ReactDOMHooks', () => {
let container;
Expand All @@ -22,6 +23,7 @@ describe('ReactDOMHooks', () => {
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');
act = require('react-dom/test-utils').unstable_concurrentAct;

container = document.createElement('div');
document.body.appendChild(container);
Expand Down Expand Up @@ -106,7 +108,7 @@ describe('ReactDOMHooks', () => {
});

// @gate experimental
it('should not bail out when an update is scheduled from within an event handler in Concurrent Mode', () => {
it('should not bail out when an update is scheduled from within an event handler in Concurrent Mode', async () => {
const {createRef, useCallback, useState} = React;

const Example = ({inputRef, labelRef}) => {
Expand All @@ -132,11 +134,14 @@ describe('ReactDOMHooks', () => {
Scheduler.unstable_flushAll();

inputRef.current.value = 'abc';
inputRef.current.dispatchEvent(
new Event('input', {bubbles: true, cancelable: true}),
);

Scheduler.unstable_flushAll();
await act(async () => {
rickhanlonii marked this conversation as resolved.
Show resolved Hide resolved
inputRef.current.dispatchEvent(
new Event('input', {
bubbles: true,
cancelable: true,
}),
);
});

expect(labelRef.current.innerHTML).toBe('abc');
});
Expand Down
8 changes: 4 additions & 4 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ function runActTests(label, render, unmount, rerender) {
expect(Scheduler).toHaveYielded([100]);
});

it('flushes effects on every call', () => {
it('flushes effects on every call', async () => {
function App() {
const [ctr, setCtr] = React.useState(0);
React.useEffect(() => {
Expand All @@ -209,16 +209,16 @@ function runActTests(label, render, unmount, rerender) {
button.dispatchEvent(new MouseEvent('click', {bubbles: true}));
}

act(() => {
await act(async () => {
click();
click();
click();
});
// it consolidates the 3 updates, then fires the effect
expect(Scheduler).toHaveYielded([3]);
act(click);
await act(async () => click());
expect(Scheduler).toHaveYielded([4]);
act(click);
await act(async () => click());
expect(Scheduler).toHaveYielded([5]);
expect(button.innerHTML).toBe('5');
});
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ describe('ChangeEventPlugin', () => {
});

// @gate experimental
it('is async for non-input events', () => {
it('is async for non-input events', async () => {
const root = ReactDOM.unstable_createRoot(container);
let input;

Expand Down
20 changes: 12 additions & 8 deletions packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe('SimpleEventPlugin', function() {
let React;
let ReactDOM;
let Scheduler;
let TestUtils;

let onClick;
let container;
Expand All @@ -39,6 +40,7 @@ describe('SimpleEventPlugin', function() {
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');
TestUtils = require('react-dom/test-utils');

onClick = jest.fn();
});
Expand Down Expand Up @@ -314,7 +316,7 @@ describe('SimpleEventPlugin', function() {
});

// @gate experimental
it('end result of many interactive updates is deterministic', () => {
it('end result of many interactive updates is deterministic', async () => {
container = document.createElement('div');
const root = ReactDOM.unstable_createRoot(container);
document.body.appendChild(container);
Expand Down Expand Up @@ -361,12 +363,14 @@ describe('SimpleEventPlugin', function() {
expect(button.textContent).toEqual('Count: 0');

// Click the button many more times
click();
click();
click();
click();
click();
click();
await TestUtils.act(async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

act should wrap click

click();
click();
click();
click();
click();
click();
});

// Flush the remaining work
Scheduler.unstable_flushAll();
Expand All @@ -375,7 +379,7 @@ describe('SimpleEventPlugin', function() {
});

// @gate experimental
it('flushes discrete updates in order', () => {
it('flushes discrete updates in order', async () => {
container = document.createElement('div');
document.body.appendChild(container);

Expand Down
48 changes: 24 additions & 24 deletions packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ describe('ReactIncrementalErrorHandling', () => {
expect(ReactNoop.getChildren()).toEqual([span('Caught an error: oops!')]);
});

it("retries at a lower priority if there's additional pending work", () => {
it("retries at a lower priority if there's additional pending work", async () => {
function App(props) {
if (props.isBroken) {
Scheduler.unstable_yieldValue('error');
Expand All @@ -252,14 +252,14 @@ describe('ReactIncrementalErrorHandling', () => {
});
}

ReactNoop.discreteUpdates(() => {
ReactNoop.render(<App isBroken={true} />, onCommit);
});
ReactNoop.render(<App isBroken={true} />, onCommit);
expect(Scheduler).toFlushAndYieldThrough(['error']);
interrupt();

// This update is in a separate batch
ReactNoop.render(<App isBroken={false} />, onCommit);
React.unstable_startTransition(() => {
// This update is in a separate batch
ReactNoop.render(<App isBroken={false} />, onCommit);
});

expect(Scheduler).toFlushAndYieldThrough([
// The first render fails. But because there's a lower priority pending
Expand Down Expand Up @@ -311,16 +311,16 @@ describe('ReactIncrementalErrorHandling', () => {
});
}

ReactNoop.discreteUpdates(() => {
ReactNoop.render(<App isBroken={true} />, onCommit);
});
ReactNoop.render(<App isBroken={true} />, onCommit);
expect(Scheduler).toFlushAndYieldThrough(['error']);
interrupt();

expect(ReactNoop).toMatchRenderedOutput(null);

// This update is in a separate batch
ReactNoop.render(<App isBroken={false} />, onCommit);
React.unstable_startTransition(() => {
// This update is in a separate batch
ReactNoop.render(<App isBroken={false} />, onCommit);
});

expect(Scheduler).toFlushAndYieldThrough([
// The first render fails. But because there's a lower priority pending
Expand Down Expand Up @@ -1789,16 +1789,17 @@ describe('ReactIncrementalErrorHandling', () => {
}

await ReactNoop.act(async () => {
ReactNoop.discreteUpdates(() => {
root.render(<Oops />);
});
root.render(<Oops />);

// Render past the component that throws, then yield.
expect(Scheduler).toFlushAndYieldThrough(['Oops']);
expect(root).toMatchRenderedOutput(null);
// Interleaved update. When the root completes, instead of throwing the
// error, it should try rendering again. This update will cause it to
// recover gracefully.
root.render('Everything is fine.');
React.unstable_startTransition(() => {
root.render('Everything is fine.');
});
});

// Should finish without throwing.
Expand Down Expand Up @@ -1829,21 +1830,20 @@ describe('ReactIncrementalErrorHandling', () => {
});

await ReactNoop.act(async () => {
// Schedule a high pri and a low pri update on the root.
ReactNoop.discreteUpdates(() => {
root.render(<Oops />);
// Schedule a default pri and a low pri update on the root.
root.render(<Oops />);
React.unstable_startTransition(() => {
root.render(<AllGood />);
});
root.render(<AllGood />);
// Render through just the high pri update. The low pri update remains on

// Render through just the default pri update. The low pri update remains on
// the queue.
expect(Scheduler).toFlushAndYieldThrough(['Everything is fine.']);

// Schedule a high pri update on a child that triggers an error.
// Schedule a default pri update on a child that triggers an error.
// The root should capture this error. But since there's still a pending
// update on the root, the error should be suppressed.
ReactNoop.discreteUpdates(() => {
setShouldThrow(true);
});
setShouldThrow(true);
});
// Should render the final state without throwing the error.
expect(Scheduler).toHaveYielded(['Everything is fine.']);
Expand Down
91 changes: 44 additions & 47 deletions packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,14 +570,13 @@ describe('ReactSuspenseWithNoopRenderer', () => {
);
}

// Schedule a high pri update and a low pri update, without rendering in
// between.
ReactNoop.discreteUpdates(() => {
// High pri
ReactNoop.render(<App />);
});
// Schedule a default pri update and a low pri update, without rendering in between.
// Default pri
ReactNoop.render(<App />);
// Low pri
ReactNoop.render(<App hide={true} />);
React.unstable_startTransition(() => {
ReactNoop.render(<App hide={true} />);
});

expect(Scheduler).toFlushAndYield([
// The first update suspends
Expand Down Expand Up @@ -1879,9 +1878,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
ReactNoop.render(<Foo />);
expect(Scheduler).toFlushAndYield(['Foo']);

ReactNoop.discreteUpdates(() =>
ReactNoop.render(<Foo renderContent={true} />),
);
ReactNoop.render(<Foo renderContent={true} />);
expect(Scheduler).toFlushAndYieldThrough(['Foo']);

// Advance some time.
Expand Down Expand Up @@ -3080,48 +3077,48 @@ describe('ReactSuspenseWithNoopRenderer', () => {
// Schedule an update inside the Suspense boundary that suspends.
setAppText('B');
expect(Scheduler).toFlushAndYield(['Suspend! [B]', 'Loading...']);
});

// Commit the placeholder
await advanceTimers(250);
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A" />
<span prop="Loading..." />
</>,
);
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A" />
<span prop="Loading..." />
</>,
);

// Schedule a high pri update on the boundary, and a lower pri update
// on the fallback. We're testing to make sure the fallback can still
// update even though the primary tree is suspended.{
ReactNoop.discreteUpdates(() => {
setAppText('C');
// Schedule a default pri update on the boundary, and a lower pri update
// on the fallback. We're testing to make sure the fallback can still
// update even though the primary tree is suspended.
await ReactNoop.act(async () => {
setAppText('C');
React.unstable_startTransition(() => {
setFallbackText('Still loading...');
});
setFallbackText('Still loading...');
});

expect(Scheduler).toFlushAndYield([
// First try to render the high pri update. Still suspended.
'Suspend! [C]',
'Loading...',
expect(Scheduler).toHaveYielded([
// First try to render the high pri update. Still suspended.
'Suspend! [C]',
'Loading...',

// In the expiration times model, once the high pri update suspends,
// we can't be sure if there's additional work at a lower priority
// that might unblock the tree. We do know that there's a lower
// priority update *somehwere* in the entire root, though (the update
// to the fallback). So we try rendering one more time, just in case.
// TODO: We shouldn't need to do this with lanes, because we always
// know exactly which lanes have pending work in each tree.
'Suspend! [C]',

// Then complete the update to the fallback.
'Still loading...',
]);
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A" />
<span prop="Still loading..." />
</>,
);
});
// In the expiration times model, once the high pri update suspends,
// we can't be sure if there's additional work at a lower priority
// that might unblock the tree. We do know that there's a lower
// priority update *somehwere* in the entire root, though (the update
// to the fallback). So we try rendering one more time, just in case.
// TODO: We shouldn't need to do this with lanes, because we always
// know exactly which lanes have pending work in each tree.
'Suspend! [C]',

// Then complete the update to the fallback.
'Still loading...',
]);
expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A" />
<span prop="Still loading..." />
</>,
);
},
);

Expand Down
9 changes: 5 additions & 4 deletions packages/react-reconciler/src/__tests__/useMutableSource-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,12 +1397,13 @@ describe('useMutableSource', () => {
// Now mutate A. Both hooks should update.
// This is at high priority so that it doesn't get batched with default
// priority updates that might fire during the passive effect
ReactNoop.discreteUpdates(() => {
mutateA('a1');
await ReactNoop.act(async () => {
ReactNoop.discreteUpdates(() => {
mutateA('a1');
});
});
expect(Scheduler).toFlushUntilNextPaint([]);

expect(root.getChildrenAsJSX()).toEqual('first: a1, second: a1');
expect(root).toMatchRenderedOutput('first: a1, second: a1');
});

expect(root.getChildrenAsJSX()).toEqual('first: a1, second: a1');
Expand Down
Loading