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.

Sign up for GitHub

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

Jump to bottom

[Flare] Update interactiveUpdates flushing heuristics #15687

Merged
merged 14 commits into from May 21, 2019
Merged

[Flare] Update interactiveUpdates flushing heuristics #15687

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
41d39be
Alter interactiveUpdates heuristics when handling events
trueadm May 20, 2019
0263165
Remove nesting layer and add test
trueadm May 20, 2019
b821081
Address feedback
trueadm May 20, 2019
5096048
Address feedback
trueadm May 20, 2019
3c2f42d
Remove .only
trueadm May 20, 2019
592a89b
Adds batchedEventUpdates and addresses feedback
trueadm May 21, 2019
773ce49
Reduce code size
trueadm May 21, 2019
6dcb4fb
Fix test in prod
trueadm May 21, 2019
9312d3d
Firefox < 53 bail out using timeStamp heuristic
trueadm May 21, 2019
228efd7
Remove console.log
trueadm May 21, 2019
f6c57af
Add TODO
trueadm May 21, 2019
eef7b78
Remove FF UA check
trueadm May 21, 2019
8050d88
Add inline message to describe intent
trueadm May 21, 2019
52ffb56
Update comment
trueadm May 21, 2019
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
36 changes: 21 additions & 15 deletions packages/events/ReactGenericBatching.js
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@ import {
needsStateRestore,
restoreStateIfNeeded,
} from './ReactControlledComponent';
import {enableEventAPI} from 'shared/ReactFeatureFlags';

// Used as a way to call batchedUpdates when we don't have a reference to
// the renderer. Such as when we're dispatching events or if third party
Expand All @@ -17,13 +18,13 @@ import {
// scheduled work and instead do synchronous work.

// Defaults
let _batchedUpdatesImpl = function(fn, bookkeeping) {
let batchedUpdatesImpl = function(fn, bookkeeping) {
return fn(bookkeeping);
};
let _interactiveUpdatesImpl = function(fn, a, b, c) {
let discreteUpdatesImpl = function(fn, a, b, c) {
return fn(a, b, c);
};
let _flushInteractiveUpdatesImpl = function() {};
let flushDiscreteUpdatesImpl = function() {};

let isBatching = false;
export function batchedUpdates(fn, bookkeeping) {
Expand All @@ -34,7 +35,7 @@ export function batchedUpdates(fn, bookkeeping) {
}
isBatching = true;
try {
return _batchedUpdatesImpl(fn, bookkeeping);
return batchedUpdatesImpl(fn, bookkeeping);
} finally {
// Here we wait until all updates have propagated, which is important
// when using controlled components within layers:
Expand All @@ -46,26 +47,31 @@ export function batchedUpdates(fn, bookkeeping) {
// 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.
_flushInteractiveUpdatesImpl();
flushDiscreteUpdatesImpl();
restoreStateIfNeeded();
}
}
}

export function interactiveUpdates(fn, a, b, c) {
return _interactiveUpdatesImpl(fn, a, b, c);
export function discreteUpdates(fn, a, b, c) {
return discreteUpdatesImpl(fn, a, b, c);
}

export function flushInteractiveUpdates() {
return _flushInteractiveUpdatesImpl();
let lastInteractiveTimeStamp = 0;
trueadm marked this conversation as resolved.
Show resolved Hide resolved

export function flushDiscreteUpdates(timeStamp?: number) {
if (!enableEventAPI || !timeStamp || lastInteractiveTimeStamp !== timeStamp) {
lastInteractiveTimeStamp = timeStamp;
return flushDiscreteUpdatesImpl();
}
}

export function setBatchingImplementation(
batchedUpdatesImpl,
interactiveUpdatesImpl,
flushInteractiveUpdatesImpl,
_batchedUpdatesImpl,
_discreteUpdatesImpl,
_flushDiscreteUpdatesImpl,
) {
_batchedUpdatesImpl = batchedUpdatesImpl;
_interactiveUpdatesImpl = interactiveUpdatesImpl;
_flushInteractiveUpdatesImpl = flushInteractiveUpdatesImpl;
batchedUpdatesImpl = _batchedUpdatesImpl;
discreteUpdatesImpl = _discreteUpdatesImpl;
flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
}
13 changes: 8 additions & 5 deletions packages/react-dom/src/client/ReactDOM.js
View file
Open in desktop
Expand Up @@ -28,8 +28,8 @@ import {
updateContainer,
batchedUpdates,
unbatchedUpdates,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushSync,
flushControlled,
injectIntoDevTools,
Expand Down Expand Up @@ -481,8 +481,8 @@ function shouldHydrateDueToLegacyHeuristic(container) {

setBatchingImplementation(
batchedUpdates,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
);

let warnedAboutHydrateAPI = false;
Expand Down Expand Up @@ -783,7 +783,10 @@ const ReactDOM: Object = {

unstable_batchedUpdates: batchedUpdates,

unstable_interactiveUpdates: interactiveUpdates,
unstable_interactiveUpdates: (fn, a, b, c) => {
flushDiscreteUpdates();
return discreteUpdates(fn, a, b, c);
},

flushSync: flushSync,

Expand Down
9 changes: 7 additions & 2 deletions packages/react-dom/src/events/DOMEventResponderSystem.js
View file
Open in desktop
Expand Up @@ -26,7 +26,11 @@ import type {
ReactResponderDispatchEventOptions,
} from 'shared/ReactTypes';
import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes';
import {batchedUpdates, interactiveUpdates} from 'events/ReactGenericBatching';
import {
batchedUpdates,
discreteUpdates,
flushDiscreteUpdates,
} from 'events/ReactGenericBatching';
import type {Fiber} from 'react-reconciler/src/ReactFiber';
import warning from 'shared/warning';
import {enableEventAPI} from 'shared/ReactFeatureFlags';
Expand Down Expand Up @@ -587,7 +591,8 @@ export function processEventQueue(): void {
return;
}
if (discrete) {
interactiveUpdates(() => {
flushDiscreteUpdates(currentTimeStamp);
discreteUpdates(() => {
batchedUpdates(processEvents, events);
});
} else {
Expand Down
18 changes: 9 additions & 9 deletions packages/react-dom/src/events/ReactDOMEventListener.js
View file
Open in desktop
Expand Up @@ -11,7 +11,11 @@ import type {AnyNativeEvent} from 'events/PluginModuleType';
import type {Fiber} from 'react-reconciler/src/ReactFiber';
import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes';

import {batchedUpdates, interactiveUpdates} from 'events/ReactGenericBatching';
import {
batchedUpdates,
discreteUpdates,
flushDiscreteUpdates,
} from 'events/ReactGenericBatching';
import {runExtractedPluginEventsInBatch} from 'events/EventPluginHub';
import {dispatchEventForResponderEventSystem} from '../events/DOMEventResponderSystem';
import {isFiberMounted} from 'react-reconciler/reflection';
Expand Down Expand Up @@ -188,7 +192,7 @@ export function trapEventForResponderEventSystem(
} else {
eventFlags |= IS_ACTIVE;
}
// Check if interactive and wrap in interactiveUpdates
// Check if interactive and wrap in discreteUpdates
const listener = dispatchEvent.bind(null, topLevelType, eventFlags);
if (passiveBrowserEventsSupported) {
addEventCaptureListenerWithPassiveFlag(
Expand All @@ -212,7 +216,7 @@ function trapEventForPluginEventSystem(
? dispatchInteractiveEvent
: dispatchEvent;
const rawEventName = getRawEventName(topLevelType);
// Check if interactive and wrap in interactiveUpdates
// Check if interactive and wrap in discreteUpdates
const listener = dispatch.bind(null, topLevelType, PLUGIN_EVENT_SYSTEM);
if (capture) {
addEventCaptureListener(element, rawEventName, listener);
Expand All @@ -222,12 +226,8 @@ function trapEventForPluginEventSystem(
}

function dispatchInteractiveEvent(topLevelType, eventSystemFlags, nativeEvent) {
interactiveUpdates(
dispatchEvent,
topLevelType,
eventSystemFlags,
nativeEvent,
);
flushDiscreteUpdates(nativeEvent.timeStamp);
discreteUpdates(dispatchEvent, topLevelType, eventSystemFlags, nativeEvent);
}

function dispatchEventForPluginEventSystem(
Expand Down
13 changes: 8 additions & 5 deletions packages/react-dom/src/fire/ReactFire.js
View file
Open in desktop
Expand Up @@ -33,8 +33,8 @@ import {
updateContainer,
batchedUpdates,
unbatchedUpdates,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushSync,
flushControlled,
injectIntoDevTools,
Expand Down Expand Up @@ -487,8 +487,8 @@ function shouldHydrateDueToLegacyHeuristic(container) {

setBatchingImplementation(
batchedUpdates,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
);

let warnedAboutHydrateAPI = false;
Expand Down Expand Up @@ -789,7 +789,10 @@ const ReactDOM: Object = {

unstable_batchedUpdates: batchedUpdates,

unstable_interactiveUpdates: interactiveUpdates,
unstable_interactiveUpdates: (fn, a, b, c) => {
flushDiscreteUpdates();
return discreteUpdates(fn, a, b, c);
},

flushSync: flushSync,

Expand Down
73 changes: 73 additions & 0 deletions packages/react-events/src/__tests__/Press-test.internal.js
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@ let React;
let ReactFeatureFlags;
let ReactDOM;
let Press;
let Scheduler;

const DEFAULT_LONG_PRESS_DELAY = 500;

Expand Down Expand Up @@ -45,6 +46,7 @@ describe('Event responder: Press', () => {
React = require('react');
ReactDOM = require('react-dom');
Press = require('react-events/press');
Scheduler = require('scheduler');

container = document.createElement('div');
document.body.appendChild(container);
Expand Down Expand Up @@ -2301,4 +2303,75 @@ describe('Event responder: Press', () => {
},
]);
});

it('should properly only flush sync once when the event systems are mixed', () => {
const ref = React.createRef();
let renderCounts = 0;

function dispatchEvent(name, timeStamp) {
const event = createEvent(name);
Object.defineProperty(event, 'timeStamp', {
value: timeStamp,
});
ref.current.dispatchEvent(event);
}

function MyComponent() {
const [, updateCounter] = React.useState(0);
renderCounts++;

function handlePress() {
updateCounter(count => count + 1);
}

return (
<div>
<Press onPress={handlePress}>
<button
ref={ref}
onClick={() => {
updateCounter(count => count + 1);
}}>
Press me
</button>
</Press>
</div>
);
}

const newContainer = document.createElement('div');
const root = ReactDOM.unstable_createRoot(newContainer);
document.body.appendChild(newContainer);
root.render(<MyComponent />);
Scheduler.flushAll();

dispatchEvent('pointerdown', 100);
dispatchEvent('pointerup', 100);
dispatchEvent('click', 100);

if (__DEV__) {
expect(renderCounts).toBe(2);
} else {
expect(renderCounts).toBe(1);
}
Scheduler.flushAll();
if (__DEV__) {
expect(renderCounts).toBe(4);
} else {
expect(renderCounts).toBe(2);
}

dispatchEvent('pointerdown', 100);
dispatchEvent('pointerup', 100);
// Ensure the timeStamp logic works
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Press event component the best place to test this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It’s fine for now. All responders should have some variant of this too.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's that the changes are being made to the core but the tests are here. And those tests have __DEV__-specific results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can follow up after and make tests in core too. I don't want to block this any longer given I'm on PTO in a few days. Maybe we can add this to our TODO list otherwise.

dispatchEvent('click', 101);

if (__DEV__) {
expect(renderCounts).toBe(6);
} else {
expect(renderCounts).toBe(3);
}

document.body.removeChild(newContainer);
});
trueadm marked this conversation as resolved.
Show resolved Hide resolved
});
8 changes: 4 additions & 4 deletions packages/react-native-renderer/src/ReactFabric.js
View file
Open in desktop
Expand Up @@ -16,8 +16,8 @@ import {
findHostInstance,
findHostInstanceWithWarning,
batchedUpdates as batchedUpdatesImpl,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
createContainer,
updateContainer,
injectIntoDevTools,
Expand Down Expand Up @@ -94,8 +94,8 @@ function findNodeHandle(componentOrHandle: any): ?number {

setBatchingImplementation(
batchedUpdatesImpl,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
);

const roots = new Map();
Expand Down
8 changes: 4 additions & 4 deletions packages/react-native-renderer/src/ReactNativeRenderer.js
View file
Open in desktop
Expand Up @@ -16,8 +16,8 @@ import {
findHostInstance,
findHostInstanceWithWarning,
batchedUpdates as batchedUpdatesImpl,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
createContainer,
updateContainer,
injectIntoDevTools,
Expand Down Expand Up @@ -99,8 +99,8 @@ function findNodeHandle(componentOrHandle: any): ?number {

setBatchingImplementation(
batchedUpdatesImpl,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
);

function computeComponentStackForErrorReporting(reactTag: number): string {
Expand Down
5 changes: 4 additions & 1 deletion packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
Expand Up @@ -1114,7 +1114,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

unbatchedUpdates: NoopRenderer.unbatchedUpdates,

interactiveUpdates: NoopRenderer.interactiveUpdates,
interactiveUpdates: <A, B, C>(fn: () => void, a: A, b: B, c: C) => {
NoopRenderer.flushDiscreteUpdates();
return NoopRenderer.discreteUpdates(fn, a, b, c);
},

flushSync(fn: () => mixed) {
NoopRenderer.flushSync(fn);
Expand Down
8 changes: 4 additions & 4 deletions packages/react-reconciler/src/ReactFiberReconciler.js
View file
Open in desktop
Expand Up @@ -52,8 +52,8 @@ import {
flushControlled,
deferredUpdates,
syncUpdates,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushPassiveEffects,
} from './ReactFiberScheduler';
import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
Expand Down Expand Up @@ -325,8 +325,8 @@ export {
unbatchedUpdates,
deferredUpdates,
syncUpdates,
interactiveUpdates,
flushInteractiveUpdates,
discreteUpdates,
flushDiscreteUpdates,
flushControlled,
flushSync,
flushPassiveEffects,
Expand Down