diff --git a/packages/events/ReactGenericBatching.js b/packages/events/ReactGenericBatching.js index 78aff1d551a1..7169b795c6da 100644 --- a/packages/events/ReactGenericBatching.js +++ b/packages/events/ReactGenericBatching.js @@ -17,15 +17,33 @@ 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 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 @@ -34,38 +52,42 @@ export function batchedUpdates(fn, bookkeeping) { } isBatching = true; try { - return _batchedUpdatesImpl(fn, bookkeeping); + return batchedUpdatesImpl(fn, bookkeeping); + } finally { + batchedUpdatesFinally(); + } +} + +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. - _flushInteractiveUpdatesImpl(); - restoreStateIfNeeded(); - } + batchedUpdatesFinally(); } } -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(); +export function flushDiscreteUpdates() { + return flushDiscreteUpdatesImpl(); } export function setBatchingImplementation( - batchedUpdatesImpl, - interactiveUpdatesImpl, - flushInteractiveUpdatesImpl, + _batchedUpdatesImpl, + _discreteUpdatesImpl, + _flushDiscreteUpdatesImpl, + _batchedEventUpdatesImpl, ) { - _batchedUpdatesImpl = batchedUpdatesImpl; - _interactiveUpdatesImpl = interactiveUpdatesImpl; - _flushInteractiveUpdatesImpl = flushInteractiveUpdatesImpl; + 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..a664ed5d9576 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js @@ -1026,9 +1026,15 @@ describe('ReactDOMFiber', () => { it('should not update event handlers until commit', () => { let ops = []; + let eventErrors = []; const handlerA = () => ops.push('A'); const handlerB = () => ops.push('B'); + spyOnProd(console, 'error'); + window.addEventListener('error', e => { + eventErrors.push(e.message); + }); + class Example extends React.Component { state = {flip: false, count: 0}; flip() { @@ -1090,12 +1096,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 011902b0450d..3629ed3e1d6a 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -26,10 +26,11 @@ import { flushRoot, createContainer, updateContainer, + batchedEventUpdates, batchedUpdates, unbatchedUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, flushSync, flushControlled, injectIntoDevTools, @@ -481,8 +482,9 @@ function shouldHydrateDueToLegacyHeuristic(container) { setBatchingImplementation( batchedUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, + batchedEventUpdates, ); let warnedAboutHydrateAPI = false; @@ -783,7 +785,14 @@ const ReactDOM: Object = { unstable_batchedUpdates: batchedUpdates, - unstable_interactiveUpdates: interactiveUpdates, + // 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, diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index 85483a24f123..1d6ecc8d25b0 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 { + batchedEventUpdates, + discreteUpdates, + flushDiscreteUpdates, +} from 'events/ReactGenericBatching'; import type {Fiber} from 'react-reconciler/src/ReactFiber'; import warning from 'shared/warning'; import {enableEventAPI} from 'shared/ReactFeatureFlags'; @@ -587,11 +591,14 @@ export function processEventQueue(): void { return; } if (discrete) { - interactiveUpdates(() => { - batchedUpdates(processEvents, events); + if (shouldflushDiscreteUpdates(currentTimeStamp)) { + flushDiscreteUpdates(); + } + discreteUpdates(() => { + batchedEventUpdates(processEvents, events); }); } else { - batchedUpdates(processEvents, events); + batchedEventUpdates(processEvents, events); } } @@ -990,3 +997,25 @@ export function generateListeningKey( const passiveKey = passive ? '_passive' : '_active'; return `${topLevelType}${passiveKey}`; } + +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 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; + } + return false; +} diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index 813f978eada1..a7749f5e8adf 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -11,9 +11,16 @@ 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 { + 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 { @@ -188,7 +195,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 +219,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 +229,10 @@ function trapEventForPluginEventSystem( } function dispatchInteractiveEvent(topLevelType, eventSystemFlags, nativeEvent) { - interactiveUpdates( - dispatchEvent, - topLevelType, - eventSystemFlags, - nativeEvent, - ); + if (!enableEventAPI || shouldflushDiscreteUpdates(nativeEvent.timeStamp)) { + flushDiscreteUpdates(); + } + discreteUpdates(dispatchEvent, topLevelType, eventSystemFlags, nativeEvent); } function dispatchEventForPluginEventSystem( @@ -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 01a71bd7cf20..6b22b7c43d67 100644 --- a/packages/react-dom/src/fire/ReactFire.js +++ b/packages/react-dom/src/fire/ReactFire.js @@ -31,10 +31,11 @@ import { flushRoot, createContainer, updateContainer, + batchedEventUpdates, batchedUpdates, unbatchedUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, flushSync, flushControlled, injectIntoDevTools, @@ -487,8 +488,9 @@ function shouldHydrateDueToLegacyHeuristic(container) { setBatchingImplementation( batchedUpdates, - interactiveUpdates, - flushInteractiveUpdates, + discreteUpdates, + flushDiscreteUpdates, + batchedEventUpdates, ); let warnedAboutHydrateAPI = false; @@ -789,7 +791,14 @@ const ReactDOM: Object = { unstable_batchedUpdates: batchedUpdates, - unstable_interactiveUpdates: interactiveUpdates, + // 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, diff --git a/packages/react-events/src/__tests__/Press-test.internal.js b/packages/react-events/src/__tests__/Press-test.internal.js index b8a5f2f58ca4..c3d73bf78ff6 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,142 @@ 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 MyComponent() { + const [, updateCounter] = React.useState(0); + renderCounts++; + + function handlePress() { + updateCounter(count => count + 1); + } + + return ( +