Skip to content

Commit

Permalink
Remove flushDiscreteUpdates from end of event (facebook#21223)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite authored and koto committed Jun 15, 2021
1 parent 9f44f22 commit 1e9f7d8
Show file tree
Hide file tree
Showing 7 changed files with 106 additions and 50 deletions.
78 changes: 78 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMNestedEvents-test.js
@@ -0,0 +1,78 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

describe('ReactDOMNestedEvents', () => {
let React;
let ReactDOM;
let Scheduler;
let TestUtils;
let act;
let useState;

beforeEach(() => {
jest.resetModules();
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');
TestUtils = require('react-dom/test-utils');
act = TestUtils.unstable_concurrentAct;
useState = React.useState;
});

// @gate experimental
test('nested event dispatches should not cause updates to flush', async () => {
const buttonRef = React.createRef(null);
function App() {
const [isClicked, setIsClicked] = useState(false);
const [isFocused, setIsFocused] = useState(false);
const onClick = () => {
setIsClicked(true);
const el = buttonRef.current;
el.focus();
// The update triggered by the focus event should not have flushed yet.
// Nor the click update. They would have if we had wrapped the focus
// call in `flushSync`, though.
Scheduler.unstable_yieldValue(
'Value right after focus call: ' + el.innerHTML,
);
};
const onFocus = () => {
setIsFocused(true);
};
return (
<>
<button ref={buttonRef} onFocus={onFocus} onClick={onClick}>
{`Clicked: ${isClicked}, Focused: ${isFocused}`}
</button>
</>
);
}

const container = document.createElement('div');
document.body.appendChild(container);
const root = ReactDOM.unstable_createRoot(container);

await act(async () => {
root.render(<App />);
});
expect(buttonRef.current.innerHTML).toEqual(
'Clicked: false, Focused: false',
);

await act(async () => {
buttonRef.current.click();
});
expect(Scheduler).toHaveYielded([
'Value right after focus call: Clicked: false, Focused: false',
]);
expect(buttonRef.current.innerHTML).toEqual('Clicked: true, Focused: true');
});
});
20 changes: 2 additions & 18 deletions packages/react-dom/src/events/ReactDOMEventListener.js
Expand Up @@ -25,21 +25,13 @@ import {
getSuspenseInstanceFromFiber,
} from 'react-reconciler/src/ReactFiberTreeReflection';
import {HostRoot, SuspenseComponent} from 'react-reconciler/src/ReactWorkTags';
import {
type EventSystemFlags,
IS_CAPTURE_PHASE,
IS_LEGACY_FB_SUPPORT_MODE,
} from './EventSystemFlags';
import {type EventSystemFlags, IS_CAPTURE_PHASE} from './EventSystemFlags';

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

import {enableLegacyFBSupport} from 'shared/ReactFeatureFlags';
import {dispatchEventForPluginEventSystem} from './DOMPluginEventSystem';
import {
flushDiscreteUpdatesIfNeeded,
discreteUpdates,
} from './ReactDOMUpdateBatching';
import {discreteUpdates} from './ReactDOMUpdateBatching';

import {
getCurrentPriorityLevel as getCurrentSchedulerPriorityLevel,
Expand Down Expand Up @@ -120,14 +112,6 @@ function dispatchDiscreteEvent(
container,
nativeEvent,
) {
if (
!enableLegacyFBSupport ||
// If we are in Legacy FB support mode, it means we've already
// flushed for this event and we don't need to do it again.
(eventSystemFlags & IS_LEGACY_FB_SUPPORT_MODE) === 0
) {
flushDiscreteUpdatesIfNeeded(nativeEvent.timeStamp);
}
discreteUpdates(
dispatchEvent,
domEventName,
Expand Down
7 changes: 0 additions & 7 deletions packages/react-dom/src/events/ReactDOMUpdateBatching.js
Expand Up @@ -88,13 +88,6 @@ export function discreteUpdates(fn, a, b, c, d) {
}
}

// TODO: Replace with flushSync
export function flushDiscreteUpdatesIfNeeded(timeStamp: number) {
if (!isInsideEventHandler) {
flushDiscreteUpdatesImpl();
}
}

export function setBatchingImplementation(
_batchedUpdatesImpl,
_discreteUpdatesImpl,
Expand Down
Expand Up @@ -656,7 +656,7 @@ describe('DOMPluginEventSystem', () => {
document.body.removeChild(parentContainer);
});

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

function Parent() {
Expand All @@ -670,7 +670,7 @@ describe('DOMPluginEventSystem', () => {
ref.current,
),
);
});
}, []);

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

ReactDOM.render(<Parent />, container);
await act(async () => {
ReactDOM.render(<Parent />, container);
});

const parent = container.lastChild;
expect(parent.id).toEqual('parent');
dispatchClickEvent(parent);

await act(async () => {
dispatchClickEvent(parent);
});

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

const child = parent.lastChild;
expect(child.id).toEqual('child');
dispatchClickEvent(child);

await act(async () => {
dispatchClickEvent(child);
});

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

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

function Parent() {
Expand All @@ -711,7 +719,7 @@ describe('DOMPluginEventSystem', () => {
ref.current,
),
);
});
}, []);

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

ReactDOM.render(<Parent />, container);
await act(async () => {
ReactDOM.render(<Parent />, container);
});

const parent = container.lastChild;
expect(parent.id).toEqual('parent');
dispatchClickEvent(parent);

await act(async () => {
dispatchClickEvent(parent);
});

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

const child = parent.lastChild;
expect(child.id).toEqual('child');
dispatchClickEvent(child);

await act(async () => {
dispatchClickEvent(child);
});

// we add both 'child' and 'parent' due to bubbling
expect(log).toEqual(['parent', 'child', 'parent']);
Expand Down
2 changes: 0 additions & 2 deletions packages/react-native-renderer/src/ReactFabric.js
Expand Up @@ -19,7 +19,6 @@ import {
batchedEventUpdates,
batchedUpdates as batchedUpdatesImpl,
discreteUpdates,
flushDiscreteUpdates,
createContainer,
updateContainer,
injectIntoDevTools,
Expand Down Expand Up @@ -242,7 +241,6 @@ function createPortal(
setBatchingImplementation(
batchedUpdatesImpl,
discreteUpdates,
flushDiscreteUpdates,
batchedEventUpdates,
);

Expand Down
2 changes: 0 additions & 2 deletions packages/react-native-renderer/src/ReactNativeRenderer.js
Expand Up @@ -19,7 +19,6 @@ import {
batchedUpdates as batchedUpdatesImpl,
batchedEventUpdates,
discreteUpdates,
flushDiscreteUpdates,
createContainer,
updateContainer,
injectIntoDevTools,
Expand Down Expand Up @@ -241,7 +240,6 @@ function createPortal(
setBatchingImplementation(
batchedUpdatesImpl,
discreteUpdates,
flushDiscreteUpdates,
batchedEventUpdates,
);

Expand Down
Expand Up @@ -18,7 +18,6 @@ let batchedUpdatesImpl = function(fn, bookkeeping) {
let discreteUpdatesImpl = function(fn, a, b, c, d) {
return fn(a, b, c, d);
};
let flushDiscreteUpdatesImpl = function() {};
let batchedEventUpdatesImpl = batchedUpdatesImpl;

let isInsideEventHandler = false;
Expand Down Expand Up @@ -59,25 +58,15 @@ export function discreteUpdates(fn, a, b, c, d) {
return discreteUpdatesImpl(fn, a, b, c, d);
} finally {
isInsideEventHandler = prevIsInsideEventHandler;
if (!isInsideEventHandler) {
}
}
}

export function flushDiscreteUpdatesIfNeeded() {
if (!isInsideEventHandler) {
flushDiscreteUpdatesImpl();
}
}

export function setBatchingImplementation(
_batchedUpdatesImpl,
_discreteUpdatesImpl,
_flushDiscreteUpdatesImpl,
_batchedEventUpdatesImpl,
) {
batchedUpdatesImpl = _batchedUpdatesImpl;
discreteUpdatesImpl = _discreteUpdatesImpl;
flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
batchedEventUpdatesImpl = _batchedEventUpdatesImpl;
}

0 comments on commit 1e9f7d8

Please sign in to comment.