Skip to content

Commit

Permalink
Don't flush sync at end of discreteUpdates
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 21, 2021
1 parent a155860 commit fb8e405
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 131 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
123 changes: 15 additions & 108 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 @@ -14,6 +14,7 @@ describe('SimpleEventPlugin', function() {
let ReactDOM;
let Scheduler;
let TestUtils;
let act;

let onClick;
let container;
Expand All @@ -40,7 +41,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 +237,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 +290,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 +314,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 +360,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 fb8e405

Please sign in to comment.