From 41d39be22c87f45fe0aa48c745613881cc49d668 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 20 May 2019 17:00:17 +0100 Subject: [PATCH 01/14] Alter interactiveUpdates heuristics when handling events --- .../src/ReactFiberScheduler.js | 36 ++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 9fd797baa10d..00063b87c5d8 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -26,6 +26,7 @@ import { disableYielding, enableSchedulerTracing, revertPassiveEffectsChange, + enableEventAPI, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -604,22 +605,41 @@ export function deferredUpdates(fn: () => A): A { return runWithPriority(NormalPriority, fn); } +let interactiveUpdatesDepth = 0; +let interactiveEventTimeStamp = 0; + export function interactiveUpdates( fn: (A, B, C) => R, a: A, b: B, c: C, ): R { - if (workPhase === NotWorking) { - // TODO: Remove this call. Instead of doing this automatically, the caller - // should explicitly call flushInteractiveUpdates. - flushPendingDiscreteUpdates(); + const currentEvent = typeof window !== 'undefined' && window.event; + let skipFlushing = enableEventAPI && interactiveUpdatesDepth !== 0; + + if (!skipFlushing && currentEvent) { + if (currentEvent.timeStamp === interactiveEventTimeStamp) { + skipFlushing = true; + } + interactiveEventTimeStamp = currentEvent.timeStamp; } - if (!revertPassiveEffectsChange) { - // TODO: Remove this call for the same reason as above. - flushPassiveEffects(); + if (!skipFlushing) { + if (workPhase === NotWorking) { + // TODO: Remove this call. Instead of doing this automatically, the caller + // should explicitly call flushInteractiveUpdates. + flushPendingDiscreteUpdates(); + } + if (!revertPassiveEffectsChange) { + // TODO: Remove this call for the same reason as above. + flushPassiveEffects(); + } + } + interactiveUpdatesDepth++; + try { + return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c)); + } finally { + interactiveUpdatesDepth--; } - return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c)); } export function syncUpdates( From 02631657aafeeb42d3ff2683f82ddfae3a04d59f Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 20 May 2019 18:54:10 +0100 Subject: [PATCH 02/14] Remove nesting layer and add test --- .../src/__tests__/Press-test.internal.js | 56 +++++++++++++++++++ .../src/ReactFiberScheduler.js | 12 +--- 2 files changed, 59 insertions(+), 9 deletions(-) diff --git a/packages/react-events/src/__tests__/Press-test.internal.js b/packages/react-events/src/__tests__/Press-test.internal.js index b8a5f2f58ca4..fd479c060c02 100644 --- a/packages/react-events/src/__tests__/Press-test.internal.js +++ b/packages/react-events/src/__tests__/Press-test.internal.js @@ -13,6 +13,7 @@ let React; let ReactFeatureFlags; let ReactDOM; let Press; +let Scheduler; const DEFAULT_LONG_PRESS_DELAY = 500; @@ -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); @@ -2301,4 +2303,58 @@ 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) { + const event = createEvent(name); + Object.defineProperty(event, 'timeStamp', { + value: 100, + }); + window.event = event; + ref.current.dispatchEvent(event); + window.event = null; + } + + function MyComponent() { + const [, updateCounter] = React.useState(0); + renderCounts++; + + function handlePress() { + updateCounter(count => count + 1); + } + + return ( +
+ + + +
+ ); + } + + const newContainer = document.createElement('div'); + const root = ReactDOM.unstable_createRoot(newContainer); + document.body.appendChild(newContainer); + root.render(); + Scheduler.flushAll(); + + dispatchEvent('pointerdown'); + dispatchEvent('pointerup'); + dispatchEvent('click'); + + expect(renderCounts).toBe(2); + Scheduler.flushAll(); + expect(renderCounts).toBe(4); + + document.body.removeChild(newContainer); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 00063b87c5d8..4843f961d9a7 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -605,7 +605,6 @@ export function deferredUpdates(fn: () => A): A { return runWithPriority(NormalPriority, fn); } -let interactiveUpdatesDepth = 0; let interactiveEventTimeStamp = 0; export function interactiveUpdates( @@ -615,9 +614,9 @@ export function interactiveUpdates( c: C, ): R { const currentEvent = typeof window !== 'undefined' && window.event; - let skipFlushing = enableEventAPI && interactiveUpdatesDepth !== 0; + let skipFlushing = false; - if (!skipFlushing && currentEvent) { + if (enableEventAPI && currentEvent) { if (currentEvent.timeStamp === interactiveEventTimeStamp) { skipFlushing = true; } @@ -634,12 +633,7 @@ export function interactiveUpdates( flushPassiveEffects(); } } - interactiveUpdatesDepth++; - try { - return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c)); - } finally { - interactiveUpdatesDepth--; - } + return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c)); } export function syncUpdates( From b821081793c314908ebb5803fd8f6d2251524be1 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 20 May 2019 20:52:27 +0100 Subject: [PATCH 03/14] Address feedback --- packages/events/ReactGenericBatching.js | 36 +++++++++++-------- packages/react-dom/src/client/ReactDOM.js | 13 ++++--- .../src/events/DOMEventResponderSystem.js | 9 +++-- .../src/events/ReactDOMEventListener.js | 18 +++++----- packages/react-dom/src/fire/ReactFire.js | 13 ++++--- .../src/__tests__/Press-test.internal.js | 35 +++++++++++++----- .../react-native-renderer/src/ReactFabric.js | 8 ++--- .../src/ReactNativeRenderer.js | 8 ++--- .../src/createReactNoop.js | 5 ++- .../src/ReactFiberReconciler.js | 8 ++--- .../src/ReactFiberScheduler.js | 31 ++++------------ 11 files changed, 101 insertions(+), 83 deletions(-) diff --git a/packages/events/ReactGenericBatching.js b/packages/events/ReactGenericBatching.js index 78aff1d551a1..9cccf1da8113 100644 --- a/packages/events/ReactGenericBatching.js +++ b/packages/events/ReactGenericBatching.js @@ -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 @@ -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) { @@ -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: @@ -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; + +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; } diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 011902b0450d..115e59198dbf 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -28,8 +28,8 @@ import { updateContainer, batchedUpdates, unbatchedUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, flushSync, flushControlled, injectIntoDevTools, @@ -481,8 +481,8 @@ function shouldHydrateDueToLegacyHeuristic(container) { setBatchingImplementation( batchedUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, ); let warnedAboutHydrateAPI = false; @@ -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, diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index 85483a24f123..8aad5bc79085 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -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'; @@ -587,7 +591,8 @@ export function processEventQueue(): void { return; } if (discrete) { - interactiveUpdates(() => { + flushDiscreteUpdates(currentTimeStamp); + discreteUpdates(() => { batchedUpdates(processEvents, events); }); } else { diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index 813f978eada1..ee957be20fcd 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -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'; @@ -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( @@ -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); @@ -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( diff --git a/packages/react-dom/src/fire/ReactFire.js b/packages/react-dom/src/fire/ReactFire.js index 01a71bd7cf20..ea646249677b 100644 --- a/packages/react-dom/src/fire/ReactFire.js +++ b/packages/react-dom/src/fire/ReactFire.js @@ -33,8 +33,8 @@ import { updateContainer, batchedUpdates, unbatchedUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, flushSync, flushControlled, injectIntoDevTools, @@ -487,8 +487,8 @@ function shouldHydrateDueToLegacyHeuristic(container) { setBatchingImplementation( batchedUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, ); let warnedAboutHydrateAPI = false; @@ -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, diff --git a/packages/react-events/src/__tests__/Press-test.internal.js b/packages/react-events/src/__tests__/Press-test.internal.js index fd479c060c02..7b39588c7148 100644 --- a/packages/react-events/src/__tests__/Press-test.internal.js +++ b/packages/react-events/src/__tests__/Press-test.internal.js @@ -2308,14 +2308,12 @@ describe('Event responder: Press', () => { const ref = React.createRef(); let renderCounts = 0; - function dispatchEvent(name) { + function dispatchEvent(name, timeStamp) { const event = createEvent(name); Object.defineProperty(event, 'timeStamp', { - value: 100, + value: timeStamp, }); - window.event = event; ref.current.dispatchEvent(event); - window.event = null; } function MyComponent() { @@ -2347,13 +2345,32 @@ describe('Event responder: Press', () => { root.render(); Scheduler.flushAll(); - dispatchEvent('pointerdown'); - dispatchEvent('pointerup'); - dispatchEvent('click'); + dispatchEvent('pointerdown', 100); + dispatchEvent('pointerup', 100); + dispatchEvent('click', 100); - expect(renderCounts).toBe(2); + if (__DEV__) { + expect(renderCounts).toBe(2); + } else { + expect(renderCounts).toBe(1); + } Scheduler.flushAll(); - expect(renderCounts).toBe(4); + if (__DEV__) { + expect(renderCounts).toBe(4); + } else { + expect(renderCounts).toBe(2); + } + + dispatchEvent('pointerdown', 100); + dispatchEvent('pointerup', 100); + // Ensure the timeStamp logic works + dispatchEvent('click', 101); + + if (__DEV__) { + expect(renderCounts).toBe(6); + } else { + expect(renderCounts).toBe(3); + } document.body.removeChild(newContainer); }); diff --git a/packages/react-native-renderer/src/ReactFabric.js b/packages/react-native-renderer/src/ReactFabric.js index 0e4bbc51d9ba..47f69352504c 100644 --- a/packages/react-native-renderer/src/ReactFabric.js +++ b/packages/react-native-renderer/src/ReactFabric.js @@ -16,8 +16,8 @@ import { findHostInstance, findHostInstanceWithWarning, batchedUpdates as batchedUpdatesImpl, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, createContainer, updateContainer, injectIntoDevTools, @@ -94,8 +94,8 @@ function findNodeHandle(componentOrHandle: any): ?number { setBatchingImplementation( batchedUpdatesImpl, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, ); const roots = new Map(); diff --git a/packages/react-native-renderer/src/ReactNativeRenderer.js b/packages/react-native-renderer/src/ReactNativeRenderer.js index 1c5bd8770742..8c99a419c27d 100644 --- a/packages/react-native-renderer/src/ReactNativeRenderer.js +++ b/packages/react-native-renderer/src/ReactNativeRenderer.js @@ -16,8 +16,8 @@ import { findHostInstance, findHostInstanceWithWarning, batchedUpdates as batchedUpdatesImpl, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, createContainer, updateContainer, injectIntoDevTools, @@ -99,8 +99,8 @@ function findNodeHandle(componentOrHandle: any): ?number { setBatchingImplementation( batchedUpdatesImpl, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, ); function computeComponentStackForErrorReporting(reactTag: number): string { diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index e7bd09890a7b..f03c2bfb813e 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -1114,7 +1114,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { unbatchedUpdates: NoopRenderer.unbatchedUpdates, - interactiveUpdates: NoopRenderer.interactiveUpdates, + interactiveUpdates: (fn: () => void, a: A, b: B, c: C) => { + NoopRenderer.flushDiscreteUpdates(); + return NoopRenderer.discreteUpdates(fn, a, b, c); + }, flushSync(fn: () => mixed) { NoopRenderer.flushSync(fn); diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index 84291f4b7a16..664ce961c764 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -52,8 +52,8 @@ import { flushControlled, deferredUpdates, syncUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, flushPassiveEffects, } from './ReactFiberScheduler'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue'; @@ -325,8 +325,8 @@ export { unbatchedUpdates, deferredUpdates, syncUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, flushControlled, flushSync, flushPassiveEffects, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 4843f961d9a7..633a9d879469 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -26,7 +26,6 @@ import { disableYielding, enableSchedulerTracing, revertPassiveEffectsChange, - enableEventAPI, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -567,8 +566,8 @@ export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) { flushSyncCallbackQueue(); } -export function flushInteractiveUpdates() { - if (workPhase === RenderPhase || workPhase === CommitPhase) { +export function flushDiscreteUpdates() { + if (workPhase !== NotWorking) { // Can't synchronously flush interactive updates if React is already // working. This is currently a no-op. // TODO: Should we fire a warning? This happens if you synchronously invoke @@ -605,33 +604,15 @@ export function deferredUpdates(fn: () => A): A { return runWithPriority(NormalPriority, fn); } -let interactiveEventTimeStamp = 0; - -export function interactiveUpdates( +export function discreteUpdates( fn: (A, B, C) => R, a: A, b: B, c: C, ): R { - const currentEvent = typeof window !== 'undefined' && window.event; - let skipFlushing = false; - - if (enableEventAPI && currentEvent) { - if (currentEvent.timeStamp === interactiveEventTimeStamp) { - skipFlushing = true; - } - interactiveEventTimeStamp = currentEvent.timeStamp; - } - if (!skipFlushing) { - if (workPhase === NotWorking) { - // TODO: Remove this call. Instead of doing this automatically, the caller - // should explicitly call flushInteractiveUpdates. - flushPendingDiscreteUpdates(); - } - if (!revertPassiveEffectsChange) { - // TODO: Remove this call for the same reason as above. - flushPassiveEffects(); - } + if (!revertPassiveEffectsChange) { + // TODO: Remove this call for the same reason as above. + flushPassiveEffects(); } return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c)); } From 5096048942ff13097669a7a97d2bdbc33cfd67f2 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 20 May 2019 23:35:08 +0100 Subject: [PATCH 04/14] Address feedback --- .../react-dom/src/__tests__/ReactTestUtilsAct-test.js | 2 +- packages/react-reconciler/src/ReactFiberScheduler.js | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index fd32092584d0..ee5bff52ab3a 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -83,7 +83,7 @@ function runActTests(label, render, unmount) { expect(Scheduler).toHaveYielded([100]); }); - it('flushes effects on every call', () => { + it.only('flushes effects on every call', () => { function App() { let [ctr, setCtr] = React.useState(0); React.useEffect(() => { diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 633a9d879469..9a3d48ca174e 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -567,7 +567,11 @@ export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) { } export function flushDiscreteUpdates() { - if (workPhase !== NotWorking) { + if ( + workPhase === RenderPhase || + workPhase === CommitPhase || + workPhase === BatchedPhase + ) { // Can't synchronously flush interactive updates if React is already // working. This is currently a no-op. // TODO: Should we fire a warning? This happens if you synchronously invoke @@ -610,10 +614,6 @@ export function discreteUpdates( b: B, c: C, ): R { - if (!revertPassiveEffectsChange) { - // TODO: Remove this call for the same reason as above. - flushPassiveEffects(); - } return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c)); } From 3c2f42d56ef4d0cd47f5bcd3e7cce269af6b2acf Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 20 May 2019 23:36:07 +0100 Subject: [PATCH 05/14] Remove .only --- packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index ee5bff52ab3a..fd32092584d0 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -83,7 +83,7 @@ function runActTests(label, render, unmount) { expect(Scheduler).toHaveYielded([100]); }); - it.only('flushes effects on every call', () => { + it('flushes effects on every call', () => { function App() { let [ctr, setCtr] = React.useState(0); React.useEffect(() => { From 592a89b45372cf9730bfa207e0c4dd9bcb6e49ea Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 01:30:25 +0100 Subject: [PATCH 06/14] Adds batchedEventUpdates and addresses feedback --- packages/events/ReactGenericBatching.js | 39 ++++++-- .../src/__tests__/ReactDOMFiber-test.js | 11 ++- packages/react-dom/src/client/ReactDOM.js | 6 ++ .../src/events/DOMEventResponderSystem.js | 20 +++- .../src/events/ReactDOMEventListener.js | 13 ++- packages/react-dom/src/fire/ReactFire.js | 6 ++ .../src/__tests__/Press-test.internal.js | 95 ++++++++++++++++--- .../react-native-renderer/src/ReactFabric.js | 2 + .../src/ReactNativeRenderer.js | 2 + .../src/createReactNoop.js | 7 +- .../src/ReactFiberReconciler.js | 2 + .../src/ReactFiberScheduler.js | 45 ++++++--- ...eactHooksWithNoopRenderer-test.internal.js | 6 +- ...tSuspenseWithNoopRenderer-test.internal.js | 6 +- 14 files changed, 209 insertions(+), 51 deletions(-) diff --git a/packages/events/ReactGenericBatching.js b/packages/events/ReactGenericBatching.js index 9cccf1da8113..3878cd257454 100644 --- a/packages/events/ReactGenericBatching.js +++ b/packages/events/ReactGenericBatching.js @@ -9,7 +9,6 @@ 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 @@ -25,6 +24,7 @@ let discreteUpdatesImpl = function(fn, a, b, c) { return fn(a, b, c); }; let flushDiscreteUpdatesImpl = function() {}; +let batchedEventUpdatesImpl = batchedUpdatesImpl; let isBatching = false; export function batchedUpdates(fn, bookkeeping) { @@ -53,25 +53,48 @@ export function batchedUpdates(fn, bookkeeping) { } } +export function batchedEventUpdates(fn, bookkeeping) { + if (isBatching) { + // If we are currently inside another batch, we need to wait until it + // fully completes before restoring state. + return fn(bookkeeping); + } + isBatching = true; + try { + return batchedEventUpdatesImpl(fn, bookkeeping); + } finally { + // Here we wait until all updates have propagated, which is important + // when using controlled components within layers: + // https://github.com/facebook/react/issues/1698 + // Then we restore state of any controlled component. + isBatching = false; + const controlledComponentsHavePendingUpdates = needsStateRestore(); + if (controlledComponentsHavePendingUpdates) { + // 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. + flushDiscreteUpdatesImpl(); + restoreStateIfNeeded(); + } + } +} + export function discreteUpdates(fn, a, b, c) { return discreteUpdatesImpl(fn, a, b, c); } -let lastInteractiveTimeStamp = 0; - -export function flushDiscreteUpdates(timeStamp?: number) { - if (!enableEventAPI || !timeStamp || lastInteractiveTimeStamp !== timeStamp) { - lastInteractiveTimeStamp = timeStamp; - return flushDiscreteUpdatesImpl(); - } +export function flushDiscreteUpdates() { + return flushDiscreteUpdatesImpl(); } export function setBatchingImplementation( _batchedUpdatesImpl, _discreteUpdatesImpl, _flushDiscreteUpdatesImpl, + _batchedEventUpdatesImpl, ) { batchedUpdatesImpl = _batchedUpdatesImpl; discreteUpdatesImpl = _discreteUpdatesImpl; flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl; + batchedEventUpdatesImpl = _batchedEventUpdatesImpl; } diff --git a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js index c19777e7e56a..4dd488c5848f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js @@ -1026,9 +1026,14 @@ describe('ReactDOMFiber', () => { it('should not update event handlers until commit', () => { let ops = []; + let eventErrors = []; const handlerA = () => ops.push('A'); const handlerB = () => ops.push('B'); + window.addEventListener('error', e => { + eventErrors.push(e.message); + }); + class Example extends React.Component { state = {flip: false, count: 0}; flip() { @@ -1090,12 +1095,16 @@ describe('ReactDOMFiber', () => { // Because the new click handler has not yet committed, we should still // invoke B. - expect(ops).toEqual(['B']); + expect(ops).toEqual([]); ops = []; // Any click that happens after commit, should invoke A. node.click(); expect(ops).toEqual(['A']); + expect(eventErrors[0]).toEqual( + 'unstable_flushDiscreteUpdates: Cannot flush ' + + 'updates when React is already rendering.', + ); }); it('should not crash encountering low-priority tree', () => { diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 115e59198dbf..3629ed3e1d6a 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -26,6 +26,7 @@ import { flushRoot, createContainer, updateContainer, + batchedEventUpdates, batchedUpdates, unbatchedUpdates, discreteUpdates, @@ -483,6 +484,7 @@ setBatchingImplementation( batchedUpdates, discreteUpdates, flushDiscreteUpdates, + batchedEventUpdates, ); let warnedAboutHydrateAPI = false; @@ -783,11 +785,15 @@ const ReactDOM: Object = { unstable_batchedUpdates: batchedUpdates, + // TODO remove this legacy method, unstable_discreteUpdates replaces it unstable_interactiveUpdates: (fn, a, b, c) => { flushDiscreteUpdates(); return discreteUpdates(fn, a, b, c); }, + unstable_discreteUpdates: discreteUpdates, + unstable_flushDiscreteUpdates: flushDiscreteUpdates, + flushSync: flushSync, unstable_createRoot: createRoot, diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index 8aad5bc79085..a2d11b5eed5d 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -27,7 +27,7 @@ import type { } from 'shared/ReactTypes'; import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes'; import { - batchedUpdates, + batchedEventUpdates, discreteUpdates, flushDiscreteUpdates, } from 'events/ReactGenericBatching'; @@ -591,12 +591,14 @@ export function processEventQueue(): void { return; } if (discrete) { - flushDiscreteUpdates(currentTimeStamp); + if (shouldflushDiscreteUpdates(currentTimeStamp)) { + flushDiscreteUpdates(); + } discreteUpdates(() => { - batchedUpdates(processEvents, events); + batchedEventUpdates(processEvents, events); }); } else { - batchedUpdates(processEvents, events); + batchedEventUpdates(processEvents, events); } } @@ -995,3 +997,13 @@ export function generateListeningKey( const passiveKey = passive ? '_passive' : '_active'; return `${topLevelType}${passiveKey}`; } + +let lastDiscreteEventTimeStamp = 0; + +export function shouldflushDiscreteUpdates(timeStamp: number): boolean { + if (lastDiscreteEventTimeStamp !== timeStamp) { + lastDiscreteEventTimeStamp = timeStamp; + return true; + } + return false; +} diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index ee957be20fcd..a7749f5e8adf 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -12,12 +12,15 @@ import type {Fiber} from 'react-reconciler/src/ReactFiber'; import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes'; import { - batchedUpdates, + batchedEventUpdates, discreteUpdates, flushDiscreteUpdates, } from 'events/ReactGenericBatching'; import {runExtractedPluginEventsInBatch} from 'events/EventPluginHub'; -import {dispatchEventForResponderEventSystem} from '../events/DOMEventResponderSystem'; +import { + dispatchEventForResponderEventSystem, + shouldflushDiscreteUpdates, +} from '../events/DOMEventResponderSystem'; import {isFiberMounted} from 'react-reconciler/reflection'; import {HostRoot} from 'shared/ReactWorkTags'; import { @@ -226,7 +229,9 @@ function trapEventForPluginEventSystem( } function dispatchInteractiveEvent(topLevelType, eventSystemFlags, nativeEvent) { - flushDiscreteUpdates(nativeEvent.timeStamp); + if (!enableEventAPI || shouldflushDiscreteUpdates(nativeEvent.timeStamp)) { + flushDiscreteUpdates(); + } discreteUpdates(dispatchEvent, topLevelType, eventSystemFlags, nativeEvent); } @@ -245,7 +250,7 @@ function dispatchEventForPluginEventSystem( try { // Event queue being processed in the same cycle allows // `preventDefault`. - batchedUpdates(handleTopLevel, bookKeeping); + batchedEventUpdates(handleTopLevel, bookKeeping); } finally { releaseTopLevelCallbackBookKeeping(bookKeeping); } diff --git a/packages/react-dom/src/fire/ReactFire.js b/packages/react-dom/src/fire/ReactFire.js index ea646249677b..6b22b7c43d67 100644 --- a/packages/react-dom/src/fire/ReactFire.js +++ b/packages/react-dom/src/fire/ReactFire.js @@ -31,6 +31,7 @@ import { flushRoot, createContainer, updateContainer, + batchedEventUpdates, batchedUpdates, unbatchedUpdates, discreteUpdates, @@ -489,6 +490,7 @@ setBatchingImplementation( batchedUpdates, discreteUpdates, flushDiscreteUpdates, + batchedEventUpdates, ); let warnedAboutHydrateAPI = false; @@ -789,11 +791,15 @@ const ReactDOM: Object = { unstable_batchedUpdates: batchedUpdates, + // TODO remove this legacy method, unstable_discreteUpdates replaces it unstable_interactiveUpdates: (fn, a, b, c) => { flushDiscreteUpdates(); return discreteUpdates(fn, a, b, c); }, + unstable_discreteUpdates: discreteUpdates, + unstable_flushDiscreteUpdates: flushDiscreteUpdates, + flushSync: flushSync, unstable_createRoot: createRoot, diff --git a/packages/react-events/src/__tests__/Press-test.internal.js b/packages/react-events/src/__tests__/Press-test.internal.js index 7b39588c7148..c3d73bf78ff6 100644 --- a/packages/react-events/src/__tests__/Press-test.internal.js +++ b/packages/react-events/src/__tests__/Press-test.internal.js @@ -2304,18 +2304,18 @@ describe('Event responder: Press', () => { ]); }); + function dispatchEventWithTimeStamp(elem, name, timeStamp) { + const event = createEvent(name); + Object.defineProperty(event, 'timeStamp', { + value: timeStamp, + }); + elem.dispatchEvent(event); + } + 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++; @@ -2345,9 +2345,9 @@ describe('Event responder: Press', () => { root.render(); Scheduler.flushAll(); - dispatchEvent('pointerdown', 100); - dispatchEvent('pointerup', 100); - dispatchEvent('click', 100); + dispatchEventWithTimeStamp(ref.current, 'pointerdown', 100); + dispatchEventWithTimeStamp(ref.current, 'pointerup', 100); + dispatchEventWithTimeStamp(ref.current, 'click', 100); if (__DEV__) { expect(renderCounts).toBe(2); @@ -2361,10 +2361,10 @@ describe('Event responder: Press', () => { expect(renderCounts).toBe(2); } - dispatchEvent('pointerdown', 100); - dispatchEvent('pointerup', 100); + dispatchEventWithTimeStamp(ref.current, 'pointerdown', 100); + dispatchEventWithTimeStamp(ref.current, 'pointerup', 100); // Ensure the timeStamp logic works - dispatchEvent('click', 101); + dispatchEventWithTimeStamp(ref.current, 'click', 101); if (__DEV__) { expect(renderCounts).toBe(6); @@ -2372,6 +2372,73 @@ describe('Event responder: Press', () => { expect(renderCounts).toBe(3); } + Scheduler.flushAll(); + document.body.removeChild(newContainer); + }); + + it('should properly flush sync when the event systems are mixed with unstable_flushDiscreteUpdates', () => { + const ref = React.createRef(); + let renderCounts = 0; + + function MyComponent() { + const [, updateCounter] = React.useState(0); + renderCounts++; + + function handlePress() { + updateCounter(count => count + 1); + } + + return ( +
+ + + +
+ ); + } + + const newContainer = document.createElement('div'); + const root = ReactDOM.unstable_createRoot(newContainer); + document.body.appendChild(newContainer); + root.render(); + Scheduler.flushAll(); + + dispatchEventWithTimeStamp(ref.current, 'pointerdown', 100); + dispatchEventWithTimeStamp(ref.current, 'pointerup', 100); + dispatchEventWithTimeStamp(ref.current, 'click', 100); + + if (__DEV__) { + expect(renderCounts).toBe(4); + } else { + expect(renderCounts).toBe(2); + } + Scheduler.flushAll(); + if (__DEV__) { + expect(renderCounts).toBe(6); + } else { + expect(renderCounts).toBe(3); + } + + dispatchEventWithTimeStamp(ref.current, 'pointerdown', 100); + dispatchEventWithTimeStamp(ref.current, 'pointerup', 100); + // Ensure the timeStamp logic works + dispatchEventWithTimeStamp(ref.current, 'click', 101); + + if (__DEV__) { + expect(renderCounts).toBe(8); + } else { + expect(renderCounts).toBe(4); + } + + Scheduler.flushAll(); document.body.removeChild(newContainer); }); }); diff --git a/packages/react-native-renderer/src/ReactFabric.js b/packages/react-native-renderer/src/ReactFabric.js index 47f69352504c..f4f499a0b7d3 100644 --- a/packages/react-native-renderer/src/ReactFabric.js +++ b/packages/react-native-renderer/src/ReactFabric.js @@ -15,6 +15,7 @@ import './ReactFabricInjection'; import { findHostInstance, findHostInstanceWithWarning, + batchedEventUpdates, batchedUpdates as batchedUpdatesImpl, discreteUpdates, flushDiscreteUpdates, @@ -96,6 +97,7 @@ setBatchingImplementation( batchedUpdatesImpl, discreteUpdates, flushDiscreteUpdates, + batchedEventUpdates, ); const roots = new Map(); diff --git a/packages/react-native-renderer/src/ReactNativeRenderer.js b/packages/react-native-renderer/src/ReactNativeRenderer.js index 8c99a419c27d..3d27d25c0ae2 100644 --- a/packages/react-native-renderer/src/ReactNativeRenderer.js +++ b/packages/react-native-renderer/src/ReactNativeRenderer.js @@ -16,6 +16,7 @@ import { findHostInstance, findHostInstanceWithWarning, batchedUpdates as batchedUpdatesImpl, + batchedEventUpdates, discreteUpdates, flushDiscreteUpdates, createContainer, @@ -101,6 +102,7 @@ setBatchingImplementation( batchedUpdatesImpl, discreteUpdates, flushDiscreteUpdates, + batchedEventUpdates, ); function computeComponentStackForErrorReporting(reactTag: number): string { diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index f03c2bfb813e..2507bc2b1da0 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -1114,10 +1114,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { unbatchedUpdates: NoopRenderer.unbatchedUpdates, - interactiveUpdates: (fn: () => void, a: A, b: B, c: C) => { - NoopRenderer.flushDiscreteUpdates(); - return NoopRenderer.discreteUpdates(fn, a, b, c); - }, + discreteUpdates: NoopRenderer.discreteUpdates, + + flushDiscreteUpdates: NoopRenderer.flushDiscreteUpdates, flushSync(fn: () => mixed) { NoopRenderer.flushSync(fn); diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index 664ce961c764..d551e9633d90 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -46,6 +46,7 @@ import { computeExpirationForFiber, scheduleWork, flushRoot, + batchedEventUpdates, batchedUpdates, unbatchedUpdates, flushSync, @@ -321,6 +322,7 @@ export function updateContainer( export { flushRoot, computeUniqueAsyncExpiration, + batchedEventUpdates, batchedUpdates, unbatchedUpdates, deferredUpdates, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 9a3d48ca174e..fe2f071275c0 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -178,13 +178,14 @@ const { ReactShouldWarnActingUpdates, } = ReactSharedInternals; -type WorkPhase = 0 | 1 | 2 | 3 | 4 | 5; +type WorkPhase = 0 | 1 | 2 | 3 | 4 | 5 | 6; const NotWorking = 0; const BatchedPhase = 1; const LegacyUnbatchedPhase = 2; const FlushSyncPhase = 3; const RenderPhase = 4; const CommitPhase = 5; +const BatchedEventPhase = 6; type RootExitStatus = 0 | 1 | 2 | 3 | 4; const RootIncomplete = 0; @@ -567,17 +568,19 @@ export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) { } export function flushDiscreteUpdates() { - if ( - workPhase === RenderPhase || - workPhase === CommitPhase || - workPhase === BatchedPhase - ) { - // Can't synchronously flush interactive updates if React is already - // working. This is currently a no-op. - // TODO: Should we fire a warning? This happens if you synchronously invoke - // an input event inside an effect, like with `element.click()`. + if (workPhase === CommitPhase || workPhase === BatchedPhase) { + // We're inside the commit phase or batched phase, so we can't + // synchronously flush pending work. This is probably a nested event + // dispatch triggered by a lifecycle/effect, like `el.focus()`. Exit. return; } + if (workPhase === RenderPhase) { + invariant( + false, + 'unstable_flushDiscreteUpdates: Cannot flush updates when React is ' + + 'already rendering.', + ); + } flushPendingDiscreteUpdates(); if (!revertPassiveEffectsChange) { // If the discrete updates scheduled passive effects, flush them now so that @@ -655,8 +658,28 @@ export function batchedUpdates(fn: A => R, a: A): R { } } +export function batchedEventUpdates(fn: A => R, a: A): R { + if (workPhase !== NotWorking) { + // We're already working, or inside a batch, so batchedUpdates is a no-op. + return fn(a); + } + const prevWorkPhase = workPhase; + workPhase = BatchedEventPhase; + try { + return fn(a); + } finally { + workPhase = prevWorkPhase; + // Flush the immediate callbacks that were scheduled during this batch + flushSyncCallbackQueue(); + } +} + export function unbatchedUpdates(fn: (a: A) => R, a: A): R { - if (workPhase !== BatchedPhase && workPhase !== FlushSyncPhase) { + if ( + workPhase !== BatchedPhase && + workPhase !== FlushSyncPhase && + workPhase !== BatchedEventPhase + ) { // We're not inside batchedUpdates or flushSync, so unbatchedUpdates is // a no-op. return fn(a); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 63081362209a..ab257d7e5623 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -885,7 +885,8 @@ describe('ReactHooksWithNoopRenderer', () => { // A discrete event forces the passive effect to be flushed -- // updateCount(1) happens first, so 2 wins. - ReactNoop.interactiveUpdates(() => { + ReactNoop.flushDiscreteUpdates(); + ReactNoop.discreteUpdates(() => { // (use batchedUpdates to silence the act() warning) ReactNoop.batchedUpdates(() => { _updateCount(2); @@ -939,7 +940,8 @@ describe('ReactHooksWithNoopRenderer', () => { // A discrete event forces the passive effect to be flushed -- // updateCount(1) happens first, so 2 wins. - ReactNoop.interactiveUpdates(() => { + ReactNoop.flushDiscreteUpdates(); + ReactNoop.discreteUpdates(() => { // use batchedUpdates to silence the act warning ReactNoop.batchedUpdates(() => _updateCount(2)); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 1bcce162f95a..fe7be360141b 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -431,7 +431,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Schedule a high pri update and a low pri update, without rendering in // between. - ReactNoop.interactiveUpdates(() => { + ReactNoop.discreteUpdates(() => { // High pri ReactNoop.render(); }); @@ -1443,7 +1443,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); } - ReactNoop.interactiveUpdates(() => ReactNoop.render()); + ReactNoop.discreteUpdates(() => ReactNoop.render()); expect(Scheduler).toFlushAndYieldThrough(['Foo']); // Advance some time. @@ -1504,7 +1504,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { ); } - ReactNoop.interactiveUpdates(() => ReactNoop.render()); + ReactNoop.discreteUpdates(() => ReactNoop.render()); Scheduler.flushAll(); // Warning is not flushed until the commit phase From 773ce49ea355c6197feed2670a63c376e3b3c393 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 01:37:01 +0100 Subject: [PATCH 07/14] Reduce code size --- packages/events/ReactGenericBatching.js | 45 +++++++++++-------------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/packages/events/ReactGenericBatching.js b/packages/events/ReactGenericBatching.js index 3878cd257454..7169b795c6da 100644 --- a/packages/events/ReactGenericBatching.js +++ b/packages/events/ReactGenericBatching.js @@ -27,6 +27,23 @@ let flushDiscreteUpdatesImpl = function() {}; let batchedEventUpdatesImpl = batchedUpdatesImpl; let isBatching = false; + +function batchedUpdatesFinally() { + // Here we wait until all updates have propagated, which is important + // when using controlled components within layers: + // https://github.com/facebook/react/issues/1698 + // Then we restore state of any controlled component. + isBatching = false; + const controlledComponentsHavePendingUpdates = needsStateRestore(); + if (controlledComponentsHavePendingUpdates) { + // 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. + flushDiscreteUpdatesImpl(); + restoreStateIfNeeded(); + } +} + export function batchedUpdates(fn, bookkeeping) { if (isBatching) { // If we are currently inside another batch, we need to wait until it @@ -37,19 +54,7 @@ export function batchedUpdates(fn, bookkeeping) { try { return batchedUpdatesImpl(fn, bookkeeping); } finally { - // Here we wait until all updates have propagated, which is important - // when using controlled components within layers: - // https://github.com/facebook/react/issues/1698 - // Then we restore state of any controlled component. - isBatching = false; - const controlledComponentsHavePendingUpdates = needsStateRestore(); - if (controlledComponentsHavePendingUpdates) { - // 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. - flushDiscreteUpdatesImpl(); - restoreStateIfNeeded(); - } + batchedUpdatesFinally(); } } @@ -63,19 +68,7 @@ export function batchedEventUpdates(fn, bookkeeping) { try { return batchedEventUpdatesImpl(fn, bookkeeping); } finally { - // Here we wait until all updates have propagated, which is important - // when using controlled components within layers: - // https://github.com/facebook/react/issues/1698 - // Then we restore state of any controlled component. - isBatching = false; - const controlledComponentsHavePendingUpdates = needsStateRestore(); - if (controlledComponentsHavePendingUpdates) { - // 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. - flushDiscreteUpdatesImpl(); - restoreStateIfNeeded(); - } + batchedUpdatesFinally(); } } From 6dcb4fb717afdf7379d15090889ae3a39bbce27a Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 01:47:38 +0100 Subject: [PATCH 08/14] Fix test in prod --- packages/react-dom/src/__tests__/ReactDOMFiber-test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js index 4dd488c5848f..a664ed5d9576 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js @@ -1030,6 +1030,7 @@ describe('ReactDOMFiber', () => { const handlerA = () => ops.push('A'); const handlerB = () => ops.push('B'); + spyOnProd(console, 'error'); window.addEventListener('error', e => { eventErrors.push(e.message); }); From 9312d3d9dec4d0d0ca53396bcbf0498a0b5bc112 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 12:44:35 +0100 Subject: [PATCH 09/14] Firefox < 53 bail out using timeStamp heuristic --- .../src/events/DOMEventResponderSystem.js | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index a2d11b5eed5d..8bde76c6e487 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -36,6 +36,7 @@ import warning from 'shared/warning'; import {enableEventAPI} from 'shared/ReactFeatureFlags'; import {invokeGuardedCallbackAndCatchFirstError} from 'shared/ReactErrorUtils'; import invariant from 'shared/invariant'; +import {canUseDOM} from 'shared/ExecutionEnvironment'; import { isFiberSuspenseAndTimedOut, getSuspenseFallbackChild, @@ -999,9 +1000,27 @@ export function generateListeningKey( } let lastDiscreteEventTimeStamp = 0; +let useTimeStampHeuristic = true; + +if (canUseDOM) { + const firefoxUA = navigator.userAgent.match(/Firefox\/(.*)$/); + + if (firefoxUA && firefoxUA.length === 2) { + const firefoxVersion = firefoxUA[1].split('.'); + if (parseInt(firefoxVersion[0], 10) < 53) { + useTimeStampHeuristic = false; + } else { + console.log(firefoxVersion[0]); + } + } +} export function shouldflushDiscreteUpdates(timeStamp: number): boolean { - if (lastDiscreteEventTimeStamp !== timeStamp) { + if ( + !useTimeStampHeuristic || + timeStamp === 0 || + lastDiscreteEventTimeStamp !== timeStamp + ) { lastDiscreteEventTimeStamp = timeStamp; return true; } From 228efd778a013fc0113dceda83a77e1b7e53f8d6 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 13:27:33 +0100 Subject: [PATCH 10/14] Remove console.log --- packages/react-dom/src/events/DOMEventResponderSystem.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index 8bde76c6e487..1bac5799990c 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -1009,8 +1009,6 @@ if (canUseDOM) { const firefoxVersion = firefoxUA[1].split('.'); if (parseInt(firefoxVersion[0], 10) < 53) { useTimeStampHeuristic = false; - } else { - console.log(firefoxVersion[0]); } } } From f6c57af4b89c29d0285aa4868d7afb8c5392cf84 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 21:09:10 +0100 Subject: [PATCH 11/14] Add TODO --- packages/react-reconciler/src/ReactFiberScheduler.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index fe2f071275c0..529c5dc98e8c 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -568,6 +568,8 @@ export function flushRoot(root: FiberRoot, expirationTime: ExpirationTime) { } export function flushDiscreteUpdates() { + // TODO: we ideally do not want to early reurn for BatchedPhase here either. + // Removing this causes act() tests to fail, so we should follow up. if (workPhase === CommitPhase || workPhase === BatchedPhase) { // We're inside the commit phase or batched phase, so we can't // synchronously flush pending work. This is probably a nested event From eef7b7800b2fea9fbd3e7891cbec558a5d79fae1 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 21:16:45 +0100 Subject: [PATCH 12/14] Remove FF UA check --- .../src/events/DOMEventResponderSystem.js | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index 1bac5799990c..5a90fd406523 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -36,7 +36,6 @@ import warning from 'shared/warning'; import {enableEventAPI} from 'shared/ReactFeatureFlags'; import {invokeGuardedCallbackAndCatchFirstError} from 'shared/ReactErrorUtils'; import invariant from 'shared/invariant'; -import {canUseDOM} from 'shared/ExecutionEnvironment'; import { isFiberSuspenseAndTimedOut, getSuspenseFallbackChild, @@ -1000,25 +999,9 @@ export function generateListeningKey( } let lastDiscreteEventTimeStamp = 0; -let useTimeStampHeuristic = true; - -if (canUseDOM) { - const firefoxUA = navigator.userAgent.match(/Firefox\/(.*)$/); - - if (firefoxUA && firefoxUA.length === 2) { - const firefoxVersion = firefoxUA[1].split('.'); - if (parseInt(firefoxVersion[0], 10) < 53) { - useTimeStampHeuristic = false; - } - } -} export function shouldflushDiscreteUpdates(timeStamp: number): boolean { - if ( - !useTimeStampHeuristic || - timeStamp === 0 || - lastDiscreteEventTimeStamp !== timeStamp - ) { + if (timeStamp === 0 || lastDiscreteEventTimeStamp !== timeStamp) { lastDiscreteEventTimeStamp = timeStamp; return true; } From 8050d880f30194fc6d5d8716a6a0aaf34d819eaa Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 21:30:21 +0100 Subject: [PATCH 13/14] Add inline message to describe intent --- .../react-dom/src/events/DOMEventResponderSystem.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index 5a90fd406523..3099a145143e 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -1001,6 +1001,16 @@ export function generateListeningKey( let lastDiscreteEventTimeStamp = 0; export function shouldflushDiscreteUpdates(timeStamp: number): boolean { + // event.timeStamp isn't overly reliable due to inconsistencies in + // how different browsers have historically provided the time stamp. + // Some browsers provide high-resolution time stamps for all events, + // some provide low-resoltion time stamps for all events. FF < 52 + // even mixes both time stamps together. Some browsers even report + // negative time stamps or time stamps that are 0 (iOS9) in some cases. + // Given we are only comparing two time stamps with equality (!==), + // we are safe from the resolution differences. If the time stamp is 0 + // we bail-out of preventing the flush, which shouldn't have any + // effect on the desired output, other than we over-flush. if (timeStamp === 0 || lastDiscreteEventTimeStamp !== timeStamp) { lastDiscreteEventTimeStamp = timeStamp; return true; From 52ffb5693cca1bbe2fd5487333b0a9aef7a0d638 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 21 May 2019 21:39:44 +0100 Subject: [PATCH 14/14] Update comment --- packages/react-dom/src/events/DOMEventResponderSystem.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index 3099a145143e..1d6ecc8d25b0 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -1009,8 +1009,10 @@ export function shouldflushDiscreteUpdates(timeStamp: number): boolean { // negative time stamps or time stamps that are 0 (iOS9) in some cases. // Given we are only comparing two time stamps with equality (!==), // we are safe from the resolution differences. If the time stamp is 0 - // we bail-out of preventing the flush, which shouldn't have any - // effect on the desired output, other than we over-flush. + // we bail-out of preventing the flush, which can affect semantics, + // such as if an earlier flush removes or adds event listeners that + // are fired in the subsequent flush. However, this is the same + // behaviour as we had before this change, so the risks are low. if (timeStamp === 0 || lastDiscreteEventTimeStamp !== timeStamp) { lastDiscreteEventTimeStamp = timeStamp; return true;