From fc807720780962729df2ad1b0f53258603c020c7 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 22 Aug 2019 23:58:16 +0100 Subject: [PATCH] [react-events] Ensure updateEventListeners updates in commit phase (#16540) --- packages/react-art/src/ReactARTHostConfig.js | 1 - .../src/client/ReactDOMHostConfig.js | 3 +- .../src/events/DOMEventResponderSystem.js | 14 +- .../DOMEventResponderSystem-test.internal.js | 76 ++++++- .../src/ReactFabricHostConfig.js | 1 - .../src/ReactNativeHostConfig.js | 1 - .../src/ReactFiberCommitWork.js | 8 + .../src/ReactFiberCompleteWork.js | 191 +--------------- .../react-reconciler/src/ReactFiberEvents.js | 214 ++++++++++++++---- .../src/ReactTestHostConfig.js | 1 - 10 files changed, 261 insertions(+), 249 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index 231ee89205be..247086c64ba2 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -432,7 +432,6 @@ export function mountResponderInstance( props: Object, state: Object, instance: Object, - rootContainerInstance: Object, ) { throw new Error('Not yet implemented.'); } diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index bc2e94f802d9..6b95e63ac9d2 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -824,10 +824,9 @@ export function mountResponderInstance( responderProps: Object, responderState: Object, instance: Instance, - rootContainerInstance: Container, ): ReactDOMEventResponderInstance { // Listen to events - const doc = rootContainerInstance.ownerDocument; + const doc = instance.ownerDocument; const documentBody = doc.body || doc; const { rootEventTypes, diff --git a/packages/react-dom/src/events/DOMEventResponderSystem.js b/packages/react-dom/src/events/DOMEventResponderSystem.js index e6db323c6349..267b9ce63b62 100644 --- a/packages/react-dom/src/events/DOMEventResponderSystem.js +++ b/packages/react-dom/src/events/DOMEventResponderSystem.js @@ -12,7 +12,7 @@ import { PASSIVE_NOT_SUPPORTED, } from 'legacy-events/EventSystemFlags'; import type {AnyNativeEvent} from 'legacy-events/PluginModuleType'; -import {HostComponent} from 'shared/ReactWorkTags'; +import {HostComponent, SuspenseComponent} from 'shared/ReactWorkTags'; import type {EventPriority} from 'shared/ReactTypes'; import type { ReactDOMEventResponder, @@ -32,10 +32,6 @@ import type {Fiber} from 'react-reconciler/src/ReactFiber'; import warning from 'shared/warning'; import {enableFlareAPI} from 'shared/ReactFeatureFlags'; import invariant from 'shared/invariant'; -import { - isFiberSuspenseAndTimedOut, - getSuspenseFallbackChild, -} from 'react-reconciler/src/ReactFiberEvents'; import {getClosestInstanceFromNode} from '../client/ReactDOMComponentTree'; import { @@ -630,6 +626,14 @@ function validateResponderContext(): void { ); } +function isFiberSuspenseAndTimedOut(fiber: Fiber): boolean { + return fiber.tag === SuspenseComponent && fiber.memoizedState !== null; +} + +function getSuspenseFallbackChild(fiber: Fiber): Fiber | null { + return ((((fiber.child: any): Fiber).sibling: any): Fiber).child; +} + export function dispatchEventForResponderEventSystem( topLevelType: string, targetFiber: null | Fiber, diff --git a/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js index 80f3a0a492b5..6eaaec450533 100644 --- a/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js +++ b/packages/react-dom/src/events/__tests__/DOMEventResponderSystem-test.internal.js @@ -14,6 +14,7 @@ let ReactFeatureFlags; let ReactDOM; let ReactDOMServer; let ReactTestRenderer; +let Scheduler; // FIXME: What should the public API be for setting an event's priority? Right // now it's an enum but is that what we want? Hard coding this for now. @@ -72,6 +73,7 @@ describe('DOMEventResponderSystem', () => { React = require('react'); ReactDOM = require('react-dom'); ReactDOMServer = require('react-dom/server'); + Scheduler = require('scheduler'); container = document.createElement('div'); document.body.appendChild(container); }); @@ -811,8 +813,8 @@ describe('DOMEventResponderSystem', () => { it('the event responder system should warn on accessing invalid properties', () => { const TestResponder = createEventResponder({ - rootEventTypes: ['click'], - onRootEvent: (event, context, props) => { + targetEventTypes: ['click'], + onEvent: (event, context, props) => { const syntheticEvent = { target: event.target, type: 'click', @@ -823,19 +825,24 @@ describe('DOMEventResponderSystem', () => { }); let handler; + let buttonRef = React.createRef(); const Test = () => { const listener = React.unstable_useResponder(TestResponder, { onClick: handler, }); - return ; + return ( + + ); }; expect(() => { handler = event => { event.preventDefault(); }; ReactDOM.render(, container); - dispatchClickEvent(document.body); + dispatchClickEvent(buttonRef.current); }).toWarnDev( 'Warning: preventDefault() is not available on event objects created from event responder modules ' + '(React Flare).' + @@ -847,7 +854,7 @@ describe('DOMEventResponderSystem', () => { event.stopPropagation(); }; ReactDOM.render(, container); - dispatchClickEvent(document.body); + dispatchClickEvent(buttonRef.current); }).toWarnDev( 'Warning: stopPropagation() is not available on event objects created from event responder modules ' + '(React Flare).' + @@ -859,7 +866,7 @@ describe('DOMEventResponderSystem', () => { event.isDefaultPrevented(); }; ReactDOM.render(, container); - dispatchClickEvent(document.body); + dispatchClickEvent(buttonRef.current); }).toWarnDev( 'Warning: isDefaultPrevented() is not available on event objects created from event responder modules ' + '(React Flare).' + @@ -871,7 +878,7 @@ describe('DOMEventResponderSystem', () => { event.isPropagationStopped(); }; ReactDOM.render(, container); - dispatchClickEvent(document.body); + dispatchClickEvent(buttonRef.current); }).toWarnDev( 'Warning: isPropagationStopped() is not available on event objects created from event responder modules ' + '(React Flare).' + @@ -883,7 +890,7 @@ describe('DOMEventResponderSystem', () => { return event.nativeEvent; }; ReactDOM.render(, container); - dispatchClickEvent(document.body); + dispatchClickEvent(buttonRef.current); }).toWarnDev( 'Warning: nativeEvent is not available on event objects created from event responder modules ' + '(React Flare).' + @@ -934,4 +941,57 @@ describe('DOMEventResponderSystem', () => { ReactDOM.render(, container); buttonRef.current.dispatchEvent(createEvent('foobar')); }); + + it('should work with concurrent mode updates', async () => { + const log = []; + const TestResponder = createEventResponder({ + targetEventTypes: ['click'], + onEvent(event, context, props) { + log.push(props); + }, + }); + const ref = React.createRef(); + + function Test({counter}) { + const listener = React.unstable_useResponder(TestResponder, {counter}); + + return ( + + ); + } + + let root = ReactDOM.unstable_createRoot(container); + let batch = root.createBatch(); + batch.render(); + Scheduler.unstable_flushAll(); + jest.runAllTimers(); + batch.commit(); + + // Click the button + dispatchClickEvent(ref.current); + expect(log).toEqual([{counter: 0}]); + + // Clear log + log.length = 0; + + // Increase counter + batch = root.createBatch(); + batch.render(); + Scheduler.unstable_flushAll(); + jest.runAllTimers(); + + // Click the button again + dispatchClickEvent(ref.current); + expect(log).toEqual([{counter: 0}]); + + // Clear log + log.length = 0; + + // Commit + batch.commit(); + dispatchClickEvent(ref.current); + expect(log).toEqual([{counter: 1}]); + }); }); diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 412e7b1094d2..977abafa9b49 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -449,7 +449,6 @@ export function mountResponderInstance( props: Object, state: Object, instance: Instance, - rootContainerInstance: Container, ) { if (enableFlareAPI) { const {rootEventTypes} = responder; diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index 8f06dff0308f..dd9e7e0e08a1 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -501,7 +501,6 @@ export function mountResponderInstance( props: Object, state: Object, instance: Instance, - rootContainerInstance: Container, ) { throw new Error('Not yet implemented.'); } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index b8996ad698ee..072e0fa8c8ed 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -118,6 +118,7 @@ import { } from './ReactHookEffectTags'; import {didWarnAboutReassigningProps} from './ReactFiberBeginWork'; import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration'; +import {updateEventListeners} from './ReactFiberEvents'; let didWarnAboutUndefinedSnapshotBeforeUpdate: Set | null = null; if (__DEV__) { @@ -1331,6 +1332,13 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { finishedWork, ); } + if (enableFlareAPI) { + const prevListeners = oldProps.listeners; + const nextListeners = newProps.listeners; + if (prevListeners !== nextListeners) { + updateEventListeners(nextListeners, instance, finishedWork); + } + } } return; } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 73d67d0e880a..f77eb30bf6aa 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -9,12 +9,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; -import type { - ReactEventResponder, - ReactEventResponderInstance, - ReactFundamentalComponentInstance, - ReactEventResponderListener, -} from 'shared/ReactTypes'; +import type {ReactFundamentalComponentInstance} from 'shared/ReactTypes'; import type {FiberRoot} from './ReactFiberRoot'; import type { Instance, @@ -31,7 +26,6 @@ import type {SuspenseContext} from './ReactFiberSuspenseContext'; import {now} from './SchedulerWithReactIntegration'; -import {REACT_RESPONDER_TYPE} from 'shared/ReactSymbols'; import { IndeterminateComponent, FunctionComponent, @@ -78,8 +72,6 @@ import { createContainerChildSet, appendChildToContainerChildSet, finalizeContainerChildren, - mountResponderInstance, - unmountResponderInstance, getFundamentalComponentInstance, mountFundamentalComponent, cloneFundamentalInstance, @@ -91,8 +83,6 @@ import { getHostContext, popHostContainer, } from './ReactFiberHostContext'; -import {NoWork} from './ReactFiberExpirationTime'; -import {createResponderInstance} from './ReactFiberEvents'; import { suspenseStackCursor, InvisibleParentSuspenseContext, @@ -132,10 +122,7 @@ import { import {createFundamentalStateInstance} from './ReactFiberFundamental'; import {Never} from './ReactFiberExpirationTime'; import {resetChildFibers} from './ReactChildFiber'; -import warning from 'shared/warning'; - -const emptyObject = {}; -const isArray = Array.isArray; +import {updateEventListeners} from './ReactFiberEvents'; function markUpdate(workInProgress: Fiber) { // Tag the fiber with an update effect. This turns a Placement into @@ -689,14 +676,8 @@ function completeWork( if (enableFlareAPI) { const prevListeners = current.memoizedProps.listeners; const nextListeners = newProps.listeners; - const instance = workInProgress.stateNode; if (prevListeners !== nextListeners) { - updateEventListeners( - nextListeners, - instance, - rootContainerInstance, - workInProgress, - ); + markUpdate(workInProgress); } } @@ -738,12 +719,7 @@ function completeWork( const instance = workInProgress.stateNode; const listeners = newProps.listeners; if (listeners != null) { - updateEventListeners( - listeners, - instance, - rootContainerInstance, - workInProgress, - ); + updateEventListeners(listeners, instance, workInProgress); } } } else { @@ -760,12 +736,7 @@ function completeWork( if (enableFlareAPI) { const listeners = newProps.listeners; if (listeners != null) { - updateEventListeners( - listeners, - instance, - rootContainerInstance, - workInProgress, - ); + updateEventListeners(listeners, instance, workInProgress); } } @@ -1253,156 +1224,4 @@ function completeWork( return null; } -function mountEventResponder( - responder: ReactEventResponder, - responderProps: Object, - instance: Instance, - rootContainerInstance: Container, - fiber: Fiber, - respondersMap: Map< - ReactEventResponder, - ReactEventResponderInstance, - >, -) { - let responderState = emptyObject; - const getInitialState = responder.getInitialState; - if (getInitialState !== null) { - responderState = getInitialState(responderProps); - } - const responderInstance = createResponderInstance( - responder, - responderProps, - responderState, - instance, - fiber, - ); - mountResponderInstance( - responder, - responderInstance, - responderProps, - responderState, - instance, - rootContainerInstance, - ); - respondersMap.set(responder, responderInstance); -} - -function updateEventListener( - listener: ReactEventResponderListener, - fiber: Fiber, - visistedResponders: Set>, - respondersMap: Map< - ReactEventResponder, - ReactEventResponderInstance, - >, - instance: Instance, - rootContainerInstance: Container, -): void { - let responder; - let props; - - if (listener) { - responder = listener.responder; - props = listener.props; - } - invariant( - responder && responder.$$typeof === REACT_RESPONDER_TYPE, - 'An invalid value was used as an event listener. Expect one or many event ' + - 'listeners created via React.unstable_useResponder().', - ); - const listenerProps = ((props: any): Object); - if (visistedResponders.has(responder)) { - // show warning - if (__DEV__) { - warning( - false, - 'Duplicate event responder "%s" found in event listeners. ' + - 'Event listeners passed to elements cannot use the same event responder more than once.', - responder.displayName, - ); - } - return; - } - visistedResponders.add(responder); - const responderInstance = respondersMap.get(responder); - - if (responderInstance === undefined) { - // Mount - mountEventResponder( - responder, - listenerProps, - instance, - rootContainerInstance, - fiber, - respondersMap, - ); - } else { - // Update - responderInstance.props = listenerProps; - responderInstance.fiber = fiber; - } -} - -function updateEventListeners( - listeners: any, - instance: Instance, - rootContainerInstance: Container, - fiber: Fiber, -): void { - const visistedResponders = new Set(); - let dependencies = fiber.dependencies; - if (listeners != null) { - if (dependencies === null) { - dependencies = fiber.dependencies = { - expirationTime: NoWork, - firstContext: null, - responders: new Map(), - }; - } - let respondersMap = dependencies.responders; - if (respondersMap === null) { - respondersMap = new Map(); - } - if (isArray(listeners)) { - for (let i = 0, length = listeners.length; i < length; i++) { - const listener = listeners[i]; - updateEventListener( - listener, - fiber, - visistedResponders, - respondersMap, - instance, - rootContainerInstance, - ); - } - } else { - updateEventListener( - listeners, - fiber, - visistedResponders, - respondersMap, - instance, - rootContainerInstance, - ); - } - } - if (dependencies !== null) { - const respondersMap = dependencies.responders; - if (respondersMap !== null) { - // Unmount - const mountedResponders = Array.from(respondersMap.keys()); - for (let i = 0, length = mountedResponders.length; i < length; i++) { - const mountedResponder = mountedResponders[i]; - if (!visistedResponders.has(mountedResponder)) { - const responderInstance = ((respondersMap.get( - mountedResponder, - ): any): ReactEventResponderInstance); - unmountResponderInstance(responderInstance); - respondersMap.delete(mountedResponder); - } - } - } - } -} - export {completeWork}; diff --git a/packages/react-reconciler/src/ReactFiberEvents.js b/packages/react-reconciler/src/ReactFiberEvents.js index a3f4404140d9..b87a11eb2ed0 100644 --- a/packages/react-reconciler/src/ReactFiberEvents.js +++ b/packages/react-reconciler/src/ReactFiberEvents.js @@ -8,59 +8,26 @@ */ import type {Fiber} from './ReactFiber'; +import type {Instance} from './ReactFiberHostConfig'; import type { ReactEventResponder, ReactEventResponderInstance, ReactEventResponderListener, } from 'shared/ReactTypes'; -import type {Instance} from './ReactFiberHostConfig'; -import {SuspenseComponent, Fragment} from 'shared/ReactWorkTags'; +import { + mountResponderInstance, + unmountResponderInstance, +} from './ReactFiberHostConfig'; +import {NoWork} from './ReactFiberExpirationTime'; -export function createResponderListener( - responder: ReactEventResponder, - props: Object, -): ReactEventResponderListener { - const eventResponderListener = { - responder, - props, - }; - if (__DEV__) { - Object.freeze(eventResponderListener); - } - return eventResponderListener; -} +import warning from 'shared/warning'; +import {REACT_RESPONDER_TYPE} from 'shared/ReactSymbols'; -export function isFiberSuspenseAndTimedOut(fiber: Fiber): boolean { - return fiber.tag === SuspenseComponent && fiber.memoizedState !== null; -} +import invariant from 'shared/invariant'; -export function getSuspenseFallbackChild(fiber: Fiber): Fiber | null { - return ((((fiber.child: any): Fiber).sibling: any): Fiber).child; -} - -export function isFiberSuspenseTimedOutChild(fiber: Fiber | null): boolean { - if (fiber === null) { - return false; - } - const parent = fiber.return; - if (parent !== null && parent.tag === Fragment) { - const grandParent = parent.return; - - if ( - grandParent !== null && - grandParent.tag === SuspenseComponent && - grandParent.stateNode !== null - ) { - return true; - } - } - return false; -} - -export function getSuspenseFiberFromTimedOutChild(fiber: Fiber): Fiber { - return ((((fiber.return: any): Fiber).return: any): Fiber); -} +const emptyObject = {}; +const isArray = Array.isArray; export function createResponderInstance( responder: ReactEventResponder, @@ -78,3 +45,162 @@ export function createResponderInstance( target, }; } + +function mountEventResponder( + responder: ReactEventResponder, + responderProps: Object, + instance: Instance, + fiber: Fiber, + respondersMap: Map< + ReactEventResponder, + ReactEventResponderInstance, + >, +) { + let responderState = emptyObject; + const getInitialState = responder.getInitialState; + if (getInitialState !== null) { + responderState = getInitialState(responderProps); + } + const responderInstance = createResponderInstance( + responder, + responderProps, + responderState, + instance, + fiber, + ); + mountResponderInstance( + responder, + responderInstance, + responderProps, + responderState, + instance, + ); + respondersMap.set(responder, responderInstance); +} + +function updateEventListener( + listener: ReactEventResponderListener, + fiber: Fiber, + visistedResponders: Set>, + respondersMap: Map< + ReactEventResponder, + ReactEventResponderInstance, + >, + instance: Instance, +): void { + let responder; + let props; + + if (listener) { + responder = listener.responder; + props = listener.props; + } + invariant( + responder && responder.$$typeof === REACT_RESPONDER_TYPE, + 'An invalid value was used as an event listener. Expect one or many event ' + + 'listeners created via React.unstable_useResponder().', + ); + const listenerProps = ((props: any): Object); + if (visistedResponders.has(responder)) { + // show warning + if (__DEV__) { + warning( + false, + 'Duplicate event responder "%s" found in event listeners. ' + + 'Event listeners passed to elements cannot use the same event responder more than once.', + responder.displayName, + ); + } + return; + } + visistedResponders.add(responder); + const responderInstance = respondersMap.get(responder); + + if (responderInstance === undefined) { + // Mount (happens in either complete or commit phase) + mountEventResponder( + responder, + listenerProps, + instance, + fiber, + respondersMap, + ); + } else { + // Update (happens during commit phase only) + responderInstance.props = listenerProps; + responderInstance.fiber = fiber; + } +} + +export function updateEventListeners( + listeners: any, + instance: Instance, + fiber: Fiber, +): void { + const visistedResponders = new Set(); + let dependencies = fiber.dependencies; + if (listeners != null) { + if (dependencies === null) { + dependencies = fiber.dependencies = { + expirationTime: NoWork, + firstContext: null, + responders: new Map(), + }; + } + let respondersMap = dependencies.responders; + if (respondersMap === null) { + respondersMap = new Map(); + } + if (isArray(listeners)) { + for (let i = 0, length = listeners.length; i < length; i++) { + const listener = listeners[i]; + updateEventListener( + listener, + fiber, + visistedResponders, + respondersMap, + instance, + ); + } + } else { + updateEventListener( + listeners, + fiber, + visistedResponders, + respondersMap, + instance, + ); + } + } + if (dependencies !== null) { + const respondersMap = dependencies.responders; + if (respondersMap !== null) { + // Unmount + const mountedResponders = Array.from(respondersMap.keys()); + for (let i = 0, length = mountedResponders.length; i < length; i++) { + const mountedResponder = mountedResponders[i]; + if (!visistedResponders.has(mountedResponder)) { + const responderInstance = ((respondersMap.get( + mountedResponder, + ): any): ReactEventResponderInstance); + unmountResponderInstance(responderInstance); + respondersMap.delete(mountedResponder); + } + } + } + } +} + +export function createResponderListener( + responder: ReactEventResponder, + props: Object, +): ReactEventResponderListener { + const eventResponderListener = { + responder, + props, + }; + if (__DEV__) { + Object.freeze(eventResponderListener); + } + return eventResponderListener; +} diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index 5f5de45dafd5..e6df01d0e726 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -296,7 +296,6 @@ export function mountResponderInstance( props: Object, state: Object, instance: Instance, - rootContainerInstance: Container, ) { // noop }