Skip to content

Commit

Permalink
Merge pull request #5940 from kentcdodds/pr/warn-event-pool-access
Browse files Browse the repository at this point in the history 
Add warning when reading from event which has been returned to the pool
  • Loading branch information
@jimfb
jimfb committed Feb 18, 2016
2 parents 5879c9e + 6312852 commit e8e56e8
Show file tree
Hide file tree
Showing 2 changed files with 127 additions and 29 deletions.
86 changes: 66 additions & 20 deletions src/renderers/dom/client/syntheticEvents/SyntheticEvent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
},

});
Expand Down Expand Up @@ -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
);
}
}
70 changes: 61 additions & 9 deletions src/renderers/dom/client/syntheticEvents/__tests__/SyntheticEvent-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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() {
Expand All @@ -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.'
);
});

Expand All @@ -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(<div onClick={assignEvent} />, 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.'
);
});
});

0 comments on commit e8e56e8

Please sign in to comment.