Skip to content

Commit

Permalink
Don't flush sync at end of discreteUpdates (#21327)
Browse files Browse the repository at this point in the history 
All it should do is change the priority. The updates will be flushed
by the microtask.
  • Loading branch information
@acdlite
acdlite committed Apr 22, 2021
1 parent a155860 commit e8cdce4
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 132 deletions.
32 changes: 20 additions & 12 deletions packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ describe('ReactDOMFiberAsync', () => {
});

// @gate experimental
it('ignores discrete events on a pending removed event listener', () => {
it('ignores discrete events on a pending removed event listener', async () => {
const disableButtonRef = React.createRef();
const submitButtonRef = React.createRef();

Expand Down Expand Up @@ -459,17 +459,19 @@ describe('ReactDOMFiberAsync', () => {
}

const root = ReactDOM.unstable_createRoot(container);
root.render(<Form />);
// Flush
Scheduler.unstable_flushAll();
await act(async () => {
root.render(<Form />);
});

const disableButton = disableButtonRef.current;
expect(disableButton.tagName).toBe('BUTTON');

// Dispatch a click event on the Disable-button.
const firstEvent = document.createEvent('Event');
firstEvent.initEvent('click', true, true);
disableButton.dispatchEvent(firstEvent);
await act(async () => {
disableButton.dispatchEvent(firstEvent);
});

// There should now be a pending update to disable the form.

Expand All @@ -481,14 +483,16 @@ describe('ReactDOMFiberAsync', () => {
const secondEvent = document.createEvent('Event');
secondEvent.initEvent('click', true, true);
// This should force the pending update to flush which disables the submit button before the event is invoked.
submitButton.dispatchEvent(secondEvent);
await act(async () => {
submitButton.dispatchEvent(secondEvent);
});

// Therefore the form should never have been submitted.
expect(formSubmitted).toBe(false);
});

// @gate experimental
it('uses the newest discrete events on a pending changed event listener', () => {
it('uses the newest discrete events on a pending changed event listener', async () => {
const enableButtonRef = React.createRef();
const submitButtonRef = React.createRef();

Expand All @@ -515,17 +519,19 @@ describe('ReactDOMFiberAsync', () => {
}

const root = ReactDOM.unstable_createRoot(container);
root.render(<Form />);
// Flush
Scheduler.unstable_flushAll();
await act(async () => {
root.render(<Form />);
});

const enableButton = enableButtonRef.current;
expect(enableButton.tagName).toBe('BUTTON');

// Dispatch a click event on the Enable-button.
const firstEvent = document.createEvent('Event');
firstEvent.initEvent('click', true, true);
enableButton.dispatchEvent(firstEvent);
await act(async () => {
enableButton.dispatchEvent(firstEvent);
});

// There should now be a pending update to enable the form.

Expand All @@ -537,7 +543,9 @@ describe('ReactDOMFiberAsync', () => {
const secondEvent = document.createEvent('Event');
secondEvent.initEvent('click', true, true);
// This should force the pending update to flush which enables the submit button before the event is invoked.
submitButton.dispatchEvent(secondEvent);
await act(async () => {
submitButton.dispatchEvent(secondEvent);
});

// Therefore the form should have been submitted.
expect(formSubmitted).toBe(true);
Expand Down
9 changes: 4 additions & 5 deletions packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,9 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
const pressEvent = document.createEvent('Event');
pressEvent.initEvent('click', true, true);
dispatchAndSetCurrentEvent(target.current, pressEvent);
// Intentionally not using `act` so we can observe in between the press
// event and the microtask, without batching.
await null;
// If this is 2, that means the `setCount` calls were not batched.
expect(container.textContent).toEqual('Count: 1');

Expand Down Expand Up @@ -409,11 +412,7 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
dispatchAndSetCurrentEvent(target, pressEvent);

expect(Scheduler).toHaveYielded(['Count: 0 [after batchedUpdates]']);
// TODO: There's a `flushDiscreteUpdates` call at the end of the event
// delegation listener that gets called even if no React event handlers are
// fired. Once that is removed, this will be 0, not 1.
// expect(container.textContent).toEqual('Count: 0');
expect(container.textContent).toEqual('Count: 1');
expect(container.textContent).toEqual('Count: 0');

// Intentionally not using `act` so we can observe in between the click
// event and the microtask, without batching.
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/events/ReactDOMUpdateBatching.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ function finishEventHandler() {
// If a controlled event was fired, we may need to restore the state of
// the DOM node back to the controlled value. This is necessary when React
// bails out of the update without touching the DOM.
// TODO: Restore state in the microtask, after the discrete updates flush,
// instead of early flushing them here.
flushDiscreteUpdatesImpl();
restoreStateIfNeeded();
}
Expand Down
124 changes: 15 additions & 109 deletions packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ describe('SimpleEventPlugin', function() {
let React;
let ReactDOM;
let Scheduler;
let TestUtils;
let act;

let onClick;
let container;
Expand All @@ -40,7 +40,6 @@ 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 @@ -237,10 +236,12 @@ describe('SimpleEventPlugin', function() {
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');

act = require('react-dom/test-utils').unstable_concurrentAct;
});

// @gate experimental
it('flushes pending interactive work before exiting event handler', () => {
it('flushes pending interactive work before exiting event handler', async () => {
container = document.createElement('div');
const root = ReactDOM.unstable_createRoot(container);
document.body.appendChild(container);
Expand Down Expand Up @@ -288,7 +289,7 @@ describe('SimpleEventPlugin', function() {
}

// Click the button to trigger the side-effect
click();
await act(async () => click());
expect(Scheduler).toHaveYielded([
// The handler fired
'Side-effect',
Expand All @@ -312,6 +313,9 @@ describe('SimpleEventPlugin', function() {
expect(Scheduler).toFlushAndYield([]);
});

// NOTE: This test was written for the old behavior of discrete updates,
// where they would be async, but flushed early if another discrete update
// was dispatched.
// @gate experimental
it('end result of many interactive updates is deterministic', async () => {
container = document.createElement('div');
Expand Down Expand Up @@ -355,121 +359,23 @@ describe('SimpleEventPlugin', function() {
}

// Click the button a single time
click();
await act(async () => click());
// The counter should update synchronously, even in concurrent mode.
expect(button.textContent).toEqual('Count: 1');

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

// Flush the remaining work
Scheduler.unstable_flushAll();
// The counter should equal the total number of clicks
expect(button.textContent).toEqual('Count: 7');
});

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

let button;
class Button extends React.Component {
state = {lowPriCount: 0};
render() {
const text = `High-pri count: ${this.props.highPriCount}, Low-pri count: ${this.state.lowPriCount}`;
Scheduler.unstable_yieldValue(text);
return (
<button
ref={el => (button = el)}
onClick={() => {
React.unstable_startTransition(() => {
this.setState(state => ({
lowPriCount: state.lowPriCount + 1,
}));
});
}}>
{text}
</button>
);
}
}

class Wrapper extends React.Component {
state = {highPriCount: 0};
render() {
return (
<div
onClick={
// Intentionally not using the updater form here, to test
// that updates are serially processed.
() => {
this.setState({highPriCount: this.state.highPriCount + 1});
}
}>
<Button highPriCount={this.state.highPriCount} />
</div>
);
}
}

// Initial mount
const root = ReactDOM.unstable_createRoot(container);
root.render(<Wrapper />);
expect(Scheduler).toFlushAndYield([
'High-pri count: 0, Low-pri count: 0',
]);
expect(button.textContent).toEqual('High-pri count: 0, Low-pri count: 0');

function click() {
const event = new MouseEvent('click', {
bubbles: true,
cancelable: true,
});
Object.defineProperty(event, 'timeStamp', {
value: 0,
});
button.dispatchEvent(event);
}

// Click the button a single time.
// This will flush at the end of the event, even in concurrent mode.
click();
expect(Scheduler).toHaveYielded(['High-pri count: 1, Low-pri count: 0']);

// Click the button many more times
click();
click();
click();
click();
click();
click();

// Each update should synchronously flush, even in concurrent mode.
expect(Scheduler).toHaveYielded([
'High-pri count: 2, Low-pri count: 0',
'High-pri count: 3, Low-pri count: 0',
'High-pri count: 4, Low-pri count: 0',
'High-pri count: 5, Low-pri count: 0',
'High-pri count: 6, Low-pri count: 0',
'High-pri count: 7, Low-pri count: 0',
]);

// Now flush the scheduler to apply the transition updates.
// At the end, both counters should equal the total number of clicks.
expect(Scheduler).toFlushAndYield([
'High-pri count: 7, Low-pri count: 7',
]);

expect(button.textContent).toEqual('High-pri count: 7, Low-pri count: 7');
});
});

describe('iOS bubbling click fix', function() {
Expand Down
3 changes: 0 additions & 3 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1160,9 +1160,6 @@ export function discreteUpdates<A, B, C, D, R>(
ReactCurrentBatchConfig.transition = prevTransition;
if (executionContext === NoContext) {
resetRenderTimer();
// TODO: This should only flush legacy sync updates. Not discrete updates
// in Concurrent Mode. Discrete updates will flush in a microtask.
flushSyncCallbacks();
}
}
}
Expand Down
3 changes: 0 additions & 3 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1160,9 +1160,6 @@ export function discreteUpdates<A, B, C, D, R>(
ReactCurrentBatchConfig.transition = prevTransition;
if (executionContext === NoContext) {
resetRenderTimer();
// TODO: This should only flush legacy sync updates. Not discrete updates
// in Concurrent Mode. Discrete updates will flush in a microtask.
flushSyncCallbacks();
}
}
}
Expand Down

0 comments on commit e8cdce4

Please sign in to comment.