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.

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

Remove flushDiscreteUpdates from end of event #21223

Merged
merged 1 commit into from Apr 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I had to change these tests because they were previously relying (accidentally) on the click event to flush the pending passive effects. Which after this change, they no longer do.

Fortunately, the fix is pretty easy. Unfortunately, this change affects legacy mode, too. I'm curious how many tests would be affected in normal practice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm. Yea that might be a big one.

});

// 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;
}