Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add warning when reading from event which has been returned to the pool #5940

Merged
merged 1 commit into from
Feb 18, 2016
Merged

Add warning when reading from event which has been returned to the pool #5940

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 66 additions & 20 deletions src/renderers/dom/client/syntheticEvents/SyntheticEvent.js
View file
Open in desktop
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) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function will be dead-code eliminated when !__DEV__ because it's only referenced in if (__DEV__) statements.

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
Open in desktop
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.'
);
});
});