Remove event pooling in the modern system (#18969)

facebook · May 21, 2020 · 4a3f779 · 4a3f779
1 parent 22f7663
commit 4a3f779
Show file tree

Hide file tree

Showing 7 changed files with 412 additions and 361 deletions.
diff --git a/packages/legacy-events/SyntheticEvent.js b/packages/legacy-events/SyntheticEvent.js
@@ -157,73 +157,79 @@ 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;
+    }
   },
 
   /**
    * Checks if this event should be released back into the pool.
    *
    * @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
@@ -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
@@ -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 <button ref={buttonRef} onClick={onClick} />;
+          }
+
+          ReactDOM.render(<Test />, container);
+
+          let buttonElement = buttonRef.current;
+          dispatchClickEvent(buttonElement);
+          expect(onClick).toHaveBeenCalledTimes(1);
+          dispatchClickEvent(buttonElement);
+          expect(onClick).toHaveBeenCalledTimes(2);
+          expect(log[0]).not.toBe(log[1]);
+          expect(log[0].type).toBe('click');
+          expect(log[1].type).toBe('click');
+        });
+
         it('handle propagation of click events', () => {
           const buttonRef = React.createRef();
           const divRef = React.createRef();

diff --git a/packages/react-dom/src/events/__tests__/SyntheticClipboardEvent-test.js b/packages/react-dom/src/events/__tests__/SyntheticClipboardEvent-test.js
@@ -11,6 +11,7 @@
 
 let React;
 let ReactDOM;
+const ReactFeatureFlags = require('shared/ReactFeatureFlags');
 
 describe('SyntheticClipboardEvent', () => {
   let container;
@@ -117,41 +118,43 @@ describe('SyntheticClipboardEvent', () => {
       expect(expectedCount).toBe(3);
     });
 
-    it('is able to `persist`', () => {
-      const persistentEvents = [];
-      const eventHandler = event => {
-        expect(event.isPersistent()).toBe(false);
-        event.persist();
-        expect(event.isPersistent()).toBe(true);
-        persistentEvents.push(event);
-      };
-
-      const div = ReactDOM.render(
-        <div
-          onCopy={eventHandler}
-          onCut={eventHandler}
-          onPaste={eventHandler}
-        />,
-        container,
-      );
-
-      let event;
-      event = document.createEvent('Event');
-      event.initEvent('copy', true, true);
-      div.dispatchEvent(event);
-
-      event = document.createEvent('Event');
-      event.initEvent('cut', true, true);
-      div.dispatchEvent(event);
-
-      event = document.createEvent('Event');
-      event.initEvent('paste', true, true);
-      div.dispatchEvent(event);
-
-      expect(persistentEvents.length).toBe(3);
-      expect(persistentEvents[0].type).toBe('copy');
-      expect(persistentEvents[1].type).toBe('cut');
-      expect(persistentEvents[2].type).toBe('paste');
-    });
+    if (!ReactFeatureFlags.enableModernEventSystem) {
+      it('is able to `persist`', () => {
+        const persistentEvents = [];
+        const eventHandler = event => {
+          expect(event.isPersistent()).toBe(false);
+          event.persist();
+          expect(event.isPersistent()).toBe(true);
+          persistentEvents.push(event);
+        };
+
+        const div = ReactDOM.render(
+          <div
+            onCopy={eventHandler}
+            onCut={eventHandler}
+            onPaste={eventHandler}
+          />,
+          container,
+        );
+
+        let event;
+        event = document.createEvent('Event');
+        event.initEvent('copy', true, true);
+        div.dispatchEvent(event);
+
+        event = document.createEvent('Event');
+        event.initEvent('cut', true, true);
+        div.dispatchEvent(event);
+
+        event = document.createEvent('Event');
+        event.initEvent('paste', true, true);
+        div.dispatchEvent(event);
+
+        expect(persistentEvents.length).toBe(3);
+        expect(persistentEvents[0].type).toBe('copy');
+        expect(persistentEvents[1].type).toBe('cut');
+        expect(persistentEvents[2].type).toBe('paste');
+      });
+    }
   });
 });