From acbb4f93f09ca37f02136f9aa6f7425fde1da2cc Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 18 Jul 2018 00:14:13 +0100 Subject: [PATCH] Remove the use of proxies for synthetic events in DEV (#13225) * Revert #5947 and disable the test * Fix isDefaultPrevented and isPropagationStopped to not get nulled This was a bug introduced by #5947. It's very confusing that they become nulled while stopPropagation/preventDefault don't. * Add a comment * Run Prettier * Fix grammar --- packages/events/SyntheticEvent.js | 84 ++++++------------- .../events/__tests__/SyntheticEvent-test.js | 82 +++++++++++++++--- 2 files changed, 95 insertions(+), 71 deletions(-) diff --git a/packages/events/SyntheticEvent.js b/packages/events/SyntheticEvent.js index 9c2d9eee1c5c8..d8335085e65f0 100644 --- a/packages/events/SyntheticEvent.js +++ b/packages/events/SyntheticEvent.js @@ -10,19 +10,8 @@ import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; -let didWarnForAddedNewProperty = false; const EVENT_POOL_SIZE = 10; -const shouldBeReleasedProperties = [ - 'dispatchConfig', - '_targetInst', - 'nativeEvent', - 'isDefaultPrevented', - 'isPropagationStopped', - '_dispatchListeners', - '_dispatchInstances', -]; - /** * @interface Event * @see http://www.w3.org/TR/DOM-Level-3-Events/ @@ -81,6 +70,8 @@ function SyntheticEvent( delete this.nativeEvent; delete this.preventDefault; delete this.stopPropagation; + delete this.isDefaultPrevented; + delete this.isPropagationStopped; } this.dispatchConfig = dispatchConfig; @@ -188,15 +179,35 @@ Object.assign(SyntheticEvent.prototype, { this[propName] = null; } } - for (let i = 0; i < shouldBeReleasedProperties.length; i++) { - this[shouldBeReleasedProperties[i]] = 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, 'nativeEvent', getPooledWarningPropertyDefinition('nativeEvent', null), ); + Object.defineProperty( + this, + 'isDefaultPrevented', + getPooledWarningPropertyDefinition( + 'isDefaultPrevented', + functionThatReturnsFalse, + ), + ); + Object.defineProperty( + this, + 'isPropagationStopped', + getPooledWarningPropertyDefinition( + 'isPropagationStopped', + functionThatReturnsFalse, + ), + ); Object.defineProperty( this, 'preventDefault', @@ -237,49 +248,6 @@ SyntheticEvent.extend = function(Interface) { return Class; }; -/** Proxying after everything set on SyntheticEvent - * to resolve Proxy issue on some WebKit browsers - * in which some Event properties are set to undefined (GH#10010) - */ -if (__DEV__) { - const isProxySupported = - typeof Proxy === 'function' && - // https://github.com/facebook/react/issues/12011 - !Object.isSealed(new Proxy({}, {})); - - if (isProxySupported) { - /*eslint-disable no-func-assign */ - SyntheticEvent = new Proxy(SyntheticEvent, { - construct: function(target, args) { - return this.apply(target, Object.create(target.prototype), args); - }, - apply: function(constructor, that, args) { - return new Proxy(constructor.apply(that, args), { - set: function(target, prop, value) { - if ( - prop !== 'isPersistent' && - !target.constructor.Interface.hasOwnProperty(prop) && - shouldBeReleasedProperties.indexOf(prop) === -1 - ) { - warningWithoutStack( - didWarnForAddedNewProperty || target.isPersistent(), - "This synthetic event is reused for performance reasons. If you're " + - "seeing this, you're adding a new property in the synthetic event object. " + - 'The property is never released. See ' + - 'https://fb.me/react-event-pooling for more information.', - ); - didWarnForAddedNewProperty = true; - } - target[prop] = value; - return true; - }, - }); - }, - }); - /*eslint-enable no-func-assign */ - } -} - addEventPoolingTo(SyntheticEvent); /** @@ -354,7 +322,7 @@ function releasePooledEvent(event) { const EventConstructor = this; invariant( event instanceof EventConstructor, - 'Trying to release an event instance into a pool of a different type.', + 'Trying to release an event instance into a pool of a different type.', ); event.destructor(); if (EventConstructor.eventPool.length < EVENT_POOL_SIZE) { diff --git a/packages/react-dom/src/events/__tests__/SyntheticEvent-test.js b/packages/react-dom/src/events/__tests__/SyntheticEvent-test.js index 6d98060e9e28a..d0049bc962713 100644 --- a/packages/react-dom/src/events/__tests__/SyntheticEvent-test.js +++ b/packages/react-dom/src/events/__tests__/SyntheticEvent-test.js @@ -249,6 +249,62 @@ describe('SyntheticEvent', () => { expect(expectedCount).toBe(1); }); + it('should warn when calling `isPropagationStopped` if the synthetic event has not been persisted', () => { + let node; + let expectedCount = 0; + let syntheticEvent; + + const eventHandler = e => { + syntheticEvent = e; + expectedCount++; + }; + node = ReactDOM.render(
, container); + + const event = document.createEvent('Event'); + event.initEvent('click', true, true); + node.dispatchEvent(event); + + expect(() => + expect(syntheticEvent.isPropagationStopped()).toBe(false), + ).toWarnDev( + 'Warning: This synthetic event is reused for performance reasons. If ' + + "you're seeing this, you're accessing the method `isPropagationStopped` on a " + + 'released/nullified synthetic event. This is a no-op function. If you must ' + + 'keep the original synthetic event around, use event.persist(). ' + + 'See https://fb.me/react-event-pooling for more information.', + {withoutStack: true}, + ); + expect(expectedCount).toBe(1); + }); + + it('should warn when calling `isDefaultPrevented` if the synthetic event has not been persisted', () => { + let node; + let expectedCount = 0; + let syntheticEvent; + + const eventHandler = e => { + syntheticEvent = e; + expectedCount++; + }; + node = ReactDOM.render(
, container); + + const event = document.createEvent('Event'); + event.initEvent('click', true, true); + node.dispatchEvent(event); + + expect(() => + expect(syntheticEvent.isDefaultPrevented()).toBe(false), + ).toWarnDev( + 'Warning: This synthetic event is reused for performance reasons. If ' + + "you're seeing this, you're accessing the method `isDefaultPrevented` on a " + + 'released/nullified synthetic event. This is a no-op function. If you must ' + + 'keep the original synthetic event around, use event.persist(). ' + + 'See https://fb.me/react-event-pooling for more information.', + {withoutStack: true}, + ); + expect(expectedCount).toBe(1); + }); + it('should properly log warnings when events simulated with rendered components', () => { let event; function assignEvent(e) { @@ -270,25 +326,25 @@ describe('SyntheticEvent', () => { ); }); - it('should warn if Proxy is supported and the synthetic event is added a property', () => { + // TODO: we might want to re-add a warning like this in the future, + // but it shouldn't use Proxies because they make debugging difficult. + // Or we might disallow this pattern altogether: + // https://github.com/facebook/react/issues/13224 + xit('should warn if a property is added to the synthetic event', () => { let node; let expectedCount = 0; let syntheticEvent; const eventHandler = e => { - if (typeof Proxy === 'function') { - expect(() => { - e.foo = 'bar'; - }).toWarnDev( - 'Warning: This synthetic event is reused for performance reasons. If ' + - "you're seeing this, you're adding a new property in the synthetic " + - 'event object. The property is never released. ' + - 'See https://fb.me/react-event-pooling for more information.', - {withoutStack: true}, - ); - } else { + expect(() => { e.foo = 'bar'; - } + }).toWarnDev( + 'Warning: This synthetic event is reused for performance reasons. If ' + + "you're seeing this, you're adding a new property in the synthetic " + + 'event object. The property is never released. ' + + 'See https://fb.me/react-event-pooling for more information.', + {withoutStack: true}, + ); syntheticEvent = e; expectedCount++; };