Skip to content

Commit 7812003

Browse files
acdliteacdlite
authored
Remove flushDiscreteUpdates from end of event (#21223)
We don't need this anymore because we flush in a microtask. This should allow us to remove the logic in the event system that tracks nested event dispatches. I added a test to confirm that nested event dispatches don't triggger a synchronous flush, like they would if we wrapped them `flushSync`. It already passed; I added it to prevent a regression.
1 parent a3a7adb commit 7812003

7 files changed

Lines changed: 106 additions & 50 deletions

File tree

‎packages/react-dom/src/__tests__/ReactDOMNestedEvents-test.js‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/**
2+
* Copyright (c) Facebook, Inc. and its affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @emails react-core
8+
*/
9+
10+
'use strict';
11+
12+
describe('ReactDOMNestedEvents', () => {
13+
let React;
14+
let ReactDOM;
15+
let Scheduler;
16+
let TestUtils;
17+
let act;
18+
let useState;
19+
20+
beforeEach(() => {
21+
jest.resetModules();
22+
React = require('react');
23+
ReactDOM = require('react-dom');
24+
Scheduler = require('scheduler');
25+
TestUtils = require('react-dom/test-utils');
26+
act = TestUtils.unstable_concurrentAct;
27+
useState = React.useState;
28+
});
29+
30+
// @gate experimental
31+
test('nested event dispatches should not cause updates to flush', async () => {
32+
const buttonRef = React.createRef(null);
33+
function App() {
34+
const [isClicked, setIsClicked] = useState(false);
35+
const [isFocused, setIsFocused] = useState(false);
36+
const onClick = () => {
37+
setIsClicked(true);
38+
const el = buttonRef.current;
39+
el.focus();
40+
// The update triggered by the focus event should not have flushed yet.
41+
// Nor the click update. They would have if we had wrapped the focus
42+
// call in `flushSync`, though.
43+
Scheduler.unstable_yieldValue(
44+
'Value right after focus call: ' + el.innerHTML,
45+
);
46+
};
47+
const onFocus = () => {
48+
setIsFocused(true);
49+
};
50+
return (
51+
<>
52+
<button ref={buttonRef} onFocus={onFocus} onClick={onClick}>
53+
{`Clicked: ${isClicked}, Focused: ${isFocused}`}
54+
</button>
55+
</>
56+
);
57+
}
58+
59+
const container = document.createElement('div');
60+
document.body.appendChild(container);
61+
const root = ReactDOM.unstable_createRoot(container);
62+
63+
await act(async () => {
64+
root.render(<App />);
65+
});
66+
expect(buttonRef.current.innerHTML).toEqual(
67+
'Clicked: false, Focused: false',
68+
);
69+
70+
await act(async () => {
71+
buttonRef.current.click();
72+
});
73+
expect(Scheduler).toHaveYielded([
74+
'Value right after focus call: Clicked: false, Focused: false',
75+
]);
76+
expect(buttonRef.current.innerHTML).toEqual('Clicked: true, Focused: true');
77+
});
78+
});

‎packages/react-dom/src/events/ReactDOMEventListener.js‎

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -25,21 +25,13 @@ import {
2525
getSuspenseInstanceFromFiber,
2626
} from 'react-reconciler/src/ReactFiberTreeReflection';
2727
import {HostRoot, SuspenseComponent} from 'react-reconciler/src/ReactWorkTags';
28-
import {
29-
type EventSystemFlags,
30-
IS_CAPTURE_PHASE,
31-
IS_LEGACY_FB_SUPPORT_MODE,
32-
} from './EventSystemFlags';
28+
import {type EventSystemFlags, IS_CAPTURE_PHASE} from './EventSystemFlags';
3329

3430
import getEventTarget from './getEventTarget';
3531
import {getClosestInstanceFromNode} from '../client/ReactDOMComponentTree';
3632

37-
import {enableLegacyFBSupport} from 'shared/ReactFeatureFlags';
3833
import {dispatchEventForPluginEventSystem} from './DOMPluginEventSystem';
39-
import {
40-
flushDiscreteUpdatesIfNeeded,
41-
discreteUpdates,
42-
} from './ReactDOMUpdateBatching';
34+
import {discreteUpdates} from './ReactDOMUpdateBatching';
4335

4436
import {
4537
getCurrentPriorityLevel as getCurrentSchedulerPriorityLevel,
@@ -120,14 +112,6 @@ function dispatchDiscreteEvent(
120112
container,
121113
nativeEvent,
122114
) {
123-
if (
124-
!enableLegacyFBSupport ||
125-
// If we are in Legacy FB support mode, it means we've already
126-
// flushed for this event and we don't need to do it again.
127-
(eventSystemFlags & IS_LEGACY_FB_SUPPORT_MODE) === 0
128-
) {
129-
flushDiscreteUpdatesIfNeeded(nativeEvent.timeStamp);
130-
}
131115
discreteUpdates(
132116
dispatchEvent,
133117
domEventName,

‎packages/react-dom/src/events/ReactDOMUpdateBatching.js‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -88,13 +88,6 @@ export function discreteUpdates(fn, a, b, c, d) {
8888
}
8989
}
9090

91-
// TODO: Replace with flushSync
92-
export function flushDiscreteUpdatesIfNeeded(timeStamp: number) {
93-
if (!isInsideEventHandler) {
94-
flushDiscreteUpdatesImpl();
95-
}
96-
}
97-
9891
export function setBatchingImplementation(
9992
_batchedUpdatesImpl,
10093
_discreteUpdatesImpl,

‎packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -656,7 +656,7 @@ describe('DOMPluginEventSystem', () => {
656656
document.body.removeChild(parentContainer);
657657
});
658658

659-
it('handle click events on dynamic portals', () => {
659+
it('handle click events on dynamic portals', async () => {
660660
const log = [];
661661

662662
function Parent() {
@@ -670,7 +670,7 @@ describe('DOMPluginEventSystem', () => {
670670
ref.current,
671671
),
672672
);
673-
});
673+
}, []);
674674

675675
return (
676676
<div ref={ref} onClick={() => log.push('parent')} id="parent">
@@ -679,25 +679,33 @@ describe('DOMPluginEventSystem', () => {
679679
);
680680
}
681681

682-
ReactDOM.render(<Parent />, container);
682+
await act(async () => {
683+
ReactDOM.render(<Parent />, container);
684+
});
683685

684686
const parent = container.lastChild;
685687
expect(parent.id).toEqual('parent');
686-
dispatchClickEvent(parent);
688+
689+
await act(async () => {
690+
dispatchClickEvent(parent);
691+
});
687692

688693
expect(log).toEqual(['parent']);
689694

690695
const child = parent.lastChild;
691696
expect(child.id).toEqual('child');
692-
dispatchClickEvent(child);
697+
698+
await act(async () => {
699+
dispatchClickEvent(child);
700+
});
693701

694702
// we add both 'child' and 'parent' due to bubbling
695703
expect(log).toEqual(['parent', 'child', 'parent']);
696704
});
697705

698706
// Slight alteration to the last test, to catch
699707
// a subtle difference in traversal.
700-
it('handle click events on dynamic portals #2', () => {
708+
it('handle click events on dynamic portals #2', async () => {
701709
const log = [];
702710

703711
function Parent() {
@@ -711,7 +719,7 @@ describe('DOMPluginEventSystem', () => {
711719
ref.current,
712720
),
713721
);
714-
});
722+
}, []);
715723

716724
return (
717725
<div ref={ref} onClick={() => log.push('parent')} id="parent">
@@ -720,17 +728,25 @@ describe('DOMPluginEventSystem', () => {
720728
);
721729
}
722730

723-
ReactDOM.render(<Parent />, container);
731+
await act(async () => {
732+
ReactDOM.render(<Parent />, container);
733+
});
724734

725735
const parent = container.lastChild;
726736
expect(parent.id).toEqual('parent');
727-
dispatchClickEvent(parent);
737+
738+
await act(async () => {
739+
dispatchClickEvent(parent);
740+
});
728741

729742
expect(log).toEqual(['parent']);
730743

731744
const child = parent.lastChild;
732745
expect(child.id).toEqual('child');
733-
dispatchClickEvent(child);
746+
747+
await act(async () => {
748+
dispatchClickEvent(child);
749+
});
734750

735751
// we add both 'child' and 'parent' due to bubbling
736752
expect(log).toEqual(['parent', 'child', 'parent']);

‎packages/react-native-renderer/src/ReactFabric.js‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import {
1919
batchedEventUpdates,
2020
batchedUpdates as batchedUpdatesImpl,
2121
discreteUpdates,
22-
flushDiscreteUpdates,
2322
createContainer,
2423
updateContainer,
2524
injectIntoDevTools,
@@ -242,7 +241,6 @@ function createPortal(
242241
setBatchingImplementation(
243242
batchedUpdatesImpl,
244243
discreteUpdates,
245-
flushDiscreteUpdates,
246244
batchedEventUpdates,
247245
);
248246

‎packages/react-native-renderer/src/ReactNativeRenderer.js‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import {
1919
batchedUpdates as batchedUpdatesImpl,
2020
batchedEventUpdates,
2121
discreteUpdates,
22-
flushDiscreteUpdates,
2322
createContainer,
2423
updateContainer,
2524
injectIntoDevTools,
@@ -241,7 +240,6 @@ function createPortal(
241240
setBatchingImplementation(
242241
batchedUpdatesImpl,
243242
discreteUpdates,
244-
flushDiscreteUpdates,
245243
batchedEventUpdates,
246244
);
247245

‎packages/react-native-renderer/src/legacy-events/ReactGenericBatching.js‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ let batchedUpdatesImpl = function(fn, bookkeeping) {
1818
let discreteUpdatesImpl = function(fn, a, b, c, d) {
1919
return fn(a, b, c, d);
2020
};
21-
let flushDiscreteUpdatesImpl = function() {};
2221
let batchedEventUpdatesImpl = batchedUpdatesImpl;
2322

2423
let isInsideEventHandler = false;
@@ -59,25 +58,15 @@ export function discreteUpdates(fn, a, b, c, d) {
5958
return discreteUpdatesImpl(fn, a, b, c, d);
6059
} finally {
6160
isInsideEventHandler = prevIsInsideEventHandler;
62-
if (!isInsideEventHandler) {
63-
}
64-
}
65-
}
66-
67-
export function flushDiscreteUpdatesIfNeeded() {
68-
if (!isInsideEventHandler) {
69-
flushDiscreteUpdatesImpl();
7061
}
7162
}
7263

7364
export function setBatchingImplementation(
7465
_batchedUpdatesImpl,
7566
_discreteUpdatesImpl,
76-
_flushDiscreteUpdatesImpl,
7767
_batchedEventUpdatesImpl,
7868
) {
7969
batchedUpdatesImpl = _batchedUpdatesImpl;
8070
discreteUpdatesImpl = _discreteUpdatesImpl;
81-
flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
8271
batchedEventUpdatesImpl = _batchedEventUpdatesImpl;
8372
}

0 commit comments

Comments
 (0)