From 212c838d035d7f50d9bcfe51a07aca9cfc28ee14 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 21 May 2020 11:53:43 +0100 Subject: [PATCH] Remove event pooling in the modern system --- packages/legacy-events/SyntheticEvent.js | 163 ++++---- .../src/events/DOMModernPluginEventSystem.js | 5 +- ...OMModernPluginEventSystem-test.internal.js | 21 + .../__tests__/SyntheticClipboardEvent-test.js | 75 ++-- .../events/__tests__/SyntheticEvent-test.js | 379 +++++++++--------- .../__tests__/SyntheticKeyboardEvent-test.js | 89 ++-- .../__tests__/SyntheticWheelEvent-test.js | 41 +- 7 files changed, 412 insertions(+), 361 deletions(-) diff --git a/packages/legacy-events/SyntheticEvent.js b/packages/legacy-events/SyntheticEvent.js index a5c8eb21a3ae8..b9ada217b0e69 100644 --- a/packages/legacy-events/SyntheticEvent.js +++ b/packages/legacy-events/SyntheticEvent.js @@ -157,7 +157,10 @@ Object.assign(SyntheticEvent.prototype, { * won't be added back into the pool. */ persist: function() { - this.isPersistent = functionThatReturnsTrue; + // Modern event system doesn't use pooling. + if (!enableModernEventSystem) { + this.isPersistent = functionThatReturnsTrue; + } }, /** @@ -165,65 +168,68 @@ Object.assign(SyntheticEvent.prototype, { * * @return {boolean} True if this should not be released, false otherwise. */ - isPersistent: functionThatReturnsFalse, + isPersistent: enableModernEventSystem + ? functionThatReturnsTrue + : functionThatReturnsFalse, /** * `PooledClass` looks for `destructor` on each instance it releases. */ destructor: function() { - const Interface = this.constructor.Interface; - for (const propName in Interface) { + // Modern event system doesn't use pooling. + if (!enableModernEventSystem) { + const Interface = this.constructor.Interface; + for (const propName in Interface) { + if (__DEV__) { + Object.defineProperty( + this, + propName, + getPooledWarningPropertyDefinition(propName, Interface[propName]), + ); + } else { + this[propName] = null; + } + } + this.dispatchConfig = null; + this._targetInst = null; + this.nativeEvent = null; + this.isDefaultPrevented = functionThatReturnsFalse; + this.isPropagationStopped = functionThatReturnsFalse; + this._dispatchListeners = null; + this._dispatchInstances = null; if (__DEV__) { Object.defineProperty( this, - propName, - getPooledWarningPropertyDefinition(propName, Interface[propName]), + 'nativeEvent', + getPooledWarningPropertyDefinition('nativeEvent', null), ); - } else { - this[propName] = null; - } - } - this.dispatchConfig = null; - this._targetInst = null; - this.nativeEvent = null; - this.isDefaultPrevented = functionThatReturnsFalse; - this.isPropagationStopped = functionThatReturnsFalse; - if (!enableModernEventSystem) { - this._dispatchListeners = null; - this._dispatchInstances = null; - } - if (__DEV__) { - Object.defineProperty( - this, - 'nativeEvent', - getPooledWarningPropertyDefinition('nativeEvent', null), - ); - Object.defineProperty( - this, - 'isDefaultPrevented', - getPooledWarningPropertyDefinition( + Object.defineProperty( + this, 'isDefaultPrevented', - functionThatReturnsFalse, - ), - ); - Object.defineProperty( - this, - 'isPropagationStopped', - getPooledWarningPropertyDefinition( + getPooledWarningPropertyDefinition( + 'isDefaultPrevented', + functionThatReturnsFalse, + ), + ); + Object.defineProperty( + this, 'isPropagationStopped', - functionThatReturnsFalse, - ), - ); - Object.defineProperty( - this, - 'preventDefault', - getPooledWarningPropertyDefinition('preventDefault', () => {}), - ); - Object.defineProperty( - this, - 'stopPropagation', - getPooledWarningPropertyDefinition('stopPropagation', () => {}), - ); + getPooledWarningPropertyDefinition( + 'isPropagationStopped', + functionThatReturnsFalse, + ), + ); + Object.defineProperty( + this, + 'preventDefault', + getPooledWarningPropertyDefinition('preventDefault', () => {}), + ); + Object.defineProperty( + this, + 'stopPropagation', + getPooledWarningPropertyDefinition('stopPropagation', () => {}), + ); + } } }, }); @@ -303,18 +309,26 @@ function getPooledWarningPropertyDefinition(propName, getVal) { } } -function getPooledEvent(dispatchConfig, targetInst, nativeEvent, nativeInst) { +function createOrGetPooledEvent( + dispatchConfig, + targetInst, + nativeEvent, + nativeInst, +) { const EventConstructor = this; - if (EventConstructor.eventPool.length) { - const instance = EventConstructor.eventPool.pop(); - EventConstructor.call( - instance, - dispatchConfig, - targetInst, - nativeEvent, - nativeInst, - ); - return instance; + // Modern event system doesn't use pooling. + if (!enableModernEventSystem) { + if (EventConstructor.eventPool.length) { + const instance = EventConstructor.eventPool.pop(); + EventConstructor.call( + instance, + dispatchConfig, + targetInst, + nativeEvent, + nativeInst, + ); + return instance; + } } return new EventConstructor( dispatchConfig, @@ -325,21 +339,28 @@ function getPooledEvent(dispatchConfig, targetInst, nativeEvent, nativeInst) { } function releasePooledEvent(event) { - const EventConstructor = this; - invariant( - event instanceof EventConstructor, - 'Trying to release an event instance into a pool of a different type.', - ); - event.destructor(); - if (EventConstructor.eventPool.length < EVENT_POOL_SIZE) { - EventConstructor.eventPool.push(event); + // Modern event system doesn't use pooling. + if (!enableModernEventSystem) { + const EventConstructor = this; + invariant( + event instanceof EventConstructor, + 'Trying to release an event instance into a pool of a different type.', + ); + event.destructor(); + if (EventConstructor.eventPool.length < EVENT_POOL_SIZE) { + EventConstructor.eventPool.push(event); + } } } function addEventPoolingTo(EventConstructor) { - EventConstructor.eventPool = []; - EventConstructor.getPooled = getPooledEvent; - EventConstructor.release = releasePooledEvent; + EventConstructor.getPooled = createOrGetPooledEvent; + + // Modern event system doesn't use pooling. + if (!enableModernEventSystem) { + EventConstructor.eventPool = []; + EventConstructor.release = releasePooledEvent; + } } export default SyntheticEvent; diff --git a/packages/react-dom/src/events/DOMModernPluginEventSystem.js b/packages/react-dom/src/events/DOMModernPluginEventSystem.js index 48f2db1b4e45e..7419686fd92b3 100644 --- a/packages/react-dom/src/events/DOMModernPluginEventSystem.js +++ b/packages/react-dom/src/events/DOMModernPluginEventSystem.js @@ -207,10 +207,7 @@ export function dispatchEventsInBatch(dispatchQueue: DispatchQueue): void { const dispatchQueueItem: DispatchQueueItem = dispatchQueue[i]; const {event, capture, bubble} = dispatchQueueItem; executeDispatchesInOrder(event, capture, bubble); - // Release the event from the pool if needed - if (!event.isPersistent()) { - event.constructor.release(event); - } + // Modern event system doesn't use pooling. } // This would be a good time to rethrow if any of the event handlers threw. rethrowCaughtError(); diff --git a/packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js index 7eeac6c7b2c96..34a13b2b6dd52 100644 --- a/packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js +++ b/packages/react-dom/src/events/__tests__/DOMModernPluginEventSystem-test.internal.js @@ -78,6 +78,27 @@ describe('DOMModernPluginEventSystem', () => { endNativeEventListenerClearDown(); }); + it('does not pool events', () => { + const buttonRef = React.createRef(); + const log = []; + const onClick = jest.fn(e => log.push(e)); + + function Test() { + return