From 631285268881f405ff4fa0d1934125110628b218 Mon Sep 17 00:00:00 2001 From: "Kent C. Dodds" Date: Fri, 29 Jan 2016 12:30:37 -0700 Subject: [PATCH] warn(SyntheticEvent): Warn when accessing or setting properties on released syntheticEvents Closes #5939 --- .../client/syntheticEvents/SyntheticEvent.js | 86 ++++++++++++++----- .../__tests__/SyntheticEvent-test.js | 70 +++++++++++++-- 2 files changed, 127 insertions(+), 29 deletions(-) diff --git a/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js b/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js index 4a6c0a841276d..f3e300cdbbba6 100644 --- a/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js +++ b/src/renderers/dom/client/syntheticEvents/SyntheticEvent.js @@ -55,6 +55,13 @@ var EventInterface = { * @param {DOMEventTarget} nativeEventTarget Target node. */ function SyntheticEvent(dispatchConfig, targetInst, nativeEvent, nativeEventTarget) { + if (__DEV__) { + // these have a getter/setter for warnings + delete this.nativeEvent; + delete this.preventDefault; + delete this.stopPropagation; + } + this.dispatchConfig = dispatchConfig; this._targetInst = targetInst; this.nativeEvent = nativeEvent; @@ -64,6 +71,9 @@ function SyntheticEvent(dispatchConfig, targetInst, nativeEvent, nativeEventTarg if (!Interface.hasOwnProperty(propName)) { continue; } + if (__DEV__) { + delete this[propName]; // this has a getter/setter for warnings + } var normalize = Interface[propName]; if (normalize) { this[propName] = normalize(nativeEvent); @@ -92,15 +102,6 @@ assign(SyntheticEvent.prototype, { preventDefault: function() { this.defaultPrevented = true; var event = this.nativeEvent; - if (__DEV__) { - warning( - event, - 'This synthetic event is reused for performance reasons. If you\'re ' + - 'seeing this, you\'re calling `preventDefault` on a ' + - 'released/nullified synthetic event. This is a no-op. See ' + - 'https://fb.me/react-event-pooling for more information.' - ); - } if (!event) { return; } @@ -115,15 +116,6 @@ assign(SyntheticEvent.prototype, { stopPropagation: function() { var event = this.nativeEvent; - if (__DEV__) { - warning( - event, - 'This synthetic event is reused for performance reasons. If you\'re ' + - 'seeing this, you\'re calling `stopPropagation` on a ' + - 'released/nullified synthetic event. This is a no-op. See ' + - 'https://fb.me/react-event-pooling for more information.' - ); - } if (!event) { return; } @@ -158,11 +150,22 @@ assign(SyntheticEvent.prototype, { destructor: function() { var Interface = this.constructor.Interface; for (var propName in Interface) { - this[propName] = null; + if (__DEV__) { + Object.defineProperty(this, propName, getPooledWarningPropertyDefinition(propName, Interface[propName])); + } else { + this[propName] = null; + } + } + if (__DEV__) { + var noop = require('emptyFunction'); + Object.defineProperty(this, 'nativeEvent', getPooledWarningPropertyDefinition('nativeEvent', null)); + Object.defineProperty(this, 'preventDefault', getPooledWarningPropertyDefinition('preventDefault', noop)); + Object.defineProperty(this, 'stopPropagation', getPooledWarningPropertyDefinition('stopPropagation', noop)); + } else { + this.nativeEvent = null; } this.dispatchConfig = null; this._targetInst = null; - this.nativeEvent = null; }, }); @@ -195,3 +198,46 @@ SyntheticEvent.augmentClass = function(Class, Interface) { PooledClass.addPoolingTo(SyntheticEvent, PooledClass.fourArgumentPooler); module.exports = SyntheticEvent; + +/** + * Helper to nullify syntheticEvent instance properties when destructing + * + * @param {object} SyntheticEvent + * @param {String} propName + * @return {object} defineProperty object + */ +function getPooledWarningPropertyDefinition(propName, getVal) { + var isFunction = typeof getVal === 'function'; + return { + configurable: true, + set: set, + get: get, + }; + + function set(val) { + var action = isFunction ? 'setting the method' : 'setting the property'; + warn(action, 'This is effectively a no-op'); + return val; + } + + function get() { + var action = isFunction ? 'accessing the method' : 'accessing the property'; + var result = isFunction ? 'This is a no-op function' : 'This is set to null'; + warn(action, result); + return getVal; + } + + function warn(action, result) { + var warningCondition = false; + warning( + warningCondition, + 'This synthetic event is reused for performance reasons. If you\'re seeing this,' + + 'you\'re %s `%s` on a released/nullified synthetic event. %s.' + + 'If you must keep the original synthetic event around, use event.persist().' + + 'See https://fb.me/react-event-pooling for more information.', + action, + propName, + result + ); + } +} diff --git a/src/renderers/dom/client/syntheticEvents/__tests__/SyntheticEvent-test.js b/src/renderers/dom/client/syntheticEvents/__tests__/SyntheticEvent-test.js index e7e3d29fe36ef..e8ffc98a83ec5 100644 --- a/src/renderers/dom/client/syntheticEvents/__tests__/SyntheticEvent-test.js +++ b/src/renderers/dom/client/syntheticEvents/__tests__/SyntheticEvent-test.js @@ -12,12 +12,18 @@ 'use strict'; var SyntheticEvent; +var React; +var ReactDOM; +var ReactTestUtils; describe('SyntheticEvent', function() { var createEvent; beforeEach(function() { SyntheticEvent = require('SyntheticEvent'); + React = require('React'); + ReactDOM = require('ReactDOM'); + ReactTestUtils = require('ReactTestUtils'); createEvent = function(nativeEvent) { var target = require('getEventTarget')(nativeEvent); @@ -72,13 +78,36 @@ describe('SyntheticEvent', function() { expect(syntheticEvent.isPersistent()).toBe(true); }); - it('should be nullified if the synthetic event has called destructor', function() { + it('should be nullified if the synthetic event has called destructor and log warnings', function() { + spyOn(console, 'error'); var target = document.createElement('div'); var syntheticEvent = createEvent({srcElement: target}); syntheticEvent.destructor(); expect(syntheticEvent.type).toBe(null); expect(syntheticEvent.nativeEvent).toBe(null); expect(syntheticEvent.target).toBe(null); + expect(console.error.calls.length).toBe(3); // once for each property accessed + expect(console.error.argsForCall[0][0]).toBe( // assert the first warning for accessing `type` + 'Warning: This synthetic event is reused for performance reasons. If you\'re seeing this,' + + 'you\'re accessing the property `type` on a released/nullified synthetic event. This is set to null.' + + 'If you must keep the original synthetic event around, use event.persist().' + + 'See https://fb.me/react-event-pooling for more information.' + ); + }); + + it('should warn when setting properties of a destructored synthetic event', function() { + spyOn(console, 'error'); + var target = document.createElement('div'); + var syntheticEvent = createEvent({srcElement: target}); + syntheticEvent.destructor(); + expect(syntheticEvent.type = 'MouseEvent').toBe('MouseEvent'); + expect(console.error.calls.length).toBe(1); + expect(console.error.argsForCall[0][0]).toBe( + 'Warning: This synthetic event is reused for performance reasons. If you\'re seeing this,' + + 'you\'re setting the property `type` on a released/nullified synthetic event. This is effectively a no-op.' + + 'If you must keep the original synthetic event around, use event.persist().' + + 'See https://fb.me/react-event-pooling for more information.' + ); }); it('should warn if the synthetic event has been released when calling `preventDefault`', function() { @@ -88,10 +117,10 @@ describe('SyntheticEvent', function() { syntheticEvent.preventDefault(); expect(console.error.calls.length).toBe(1); expect(console.error.argsForCall[0][0]).toBe( - 'Warning: This synthetic event is reused for performance reasons. If ' + - 'you\'re seeing this, you\'re calling `preventDefault` on a ' + - 'released/nullified synthetic event. This is a no-op. See ' + - 'https://fb.me/react-event-pooling for more information.' + 'Warning: This synthetic event is reused for performance reasons. If you\'re seeing this,' + + 'you\'re accessing the method `preventDefault` 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.' ); }); @@ -102,10 +131,33 @@ describe('SyntheticEvent', function() { syntheticEvent.stopPropagation(); expect(console.error.calls.length).toBe(1); expect(console.error.argsForCall[0][0]).toBe( - 'Warning: This synthetic event is reused for performance reasons. If ' + - 'you\'re seeing this, you\'re calling `stopPropagation` on a ' + - 'released/nullified synthetic event. This is a no-op. See ' + - 'https://fb.me/react-event-pooling for more information.' + 'Warning: This synthetic event is reused for performance reasons. If you\'re seeing this,' + + 'you\'re accessing the method `stopPropagation` 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.' + ); + }); + + it('should properly log warnings when events simulated with rendered components', function() { + spyOn(console, 'error'); + var event; + var element = document.createElement('div'); + function assignEvent(e) { + event = e; + } + var instance = ReactDOM.render(
, element); + ReactTestUtils.Simulate.click(ReactDOM.findDOMNode(instance)); + expect(console.error.calls.length).toBe(0); + + // access a property to cause the warning + event.nativeEvent; // eslint-disable-line no-unused-expressions + + expect(console.error.calls.length).toBe(1); + expect(console.error.argsForCall[0][0]).toBe( + 'Warning: This synthetic event is reused for performance reasons. If you\'re seeing this,' + + 'you\'re accessing the property `nativeEvent` on a released/nullified synthetic event. This is set to null.' + + 'If you must keep the original synthetic event around, use event.persist().' + + 'See https://fb.me/react-event-pooling for more information.' ); }); });