Skip to content

Commit

Permalink
warn(SyntheticEvent): Warn when accessing or setting properties on re…
Browse files Browse the repository at this point in the history
…leased syntheticEvents

Closes #5939
  • Loading branch information
Kent C. Dodds committed Jan 30, 2016
1 parent 9c3f595 commit 69770b3
Show file tree
Hide file tree
Showing 2 changed files with 102 additions and 28 deletions.
83 changes: 63 additions & 20 deletions src/renderers/dom/client/syntheticEvents/SyntheticEvent.js
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,43 @@ 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) {
return {
configurable: true,
set: set,
get: get,
};

function set(val) {
warn('setting', 'This is effectively a no-op');
return val;
}

function get() {
var result = typeof getVal === 'function' ? 'This is a no-op function' : 'This is set to null';
warn('accessing', 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 property `%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
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ describe('SyntheticEvent', function() {
});

it('should be nullified if the synthetic event has called destructor', function() {
spyOn(console, 'error'); // accessing properties on destructored events logs warnings (tested elsewhere)
var target = document.createElement('div');
var syntheticEvent = createEvent({srcElement: target});
syntheticEvent.destructor();
Expand All @@ -81,17 +82,47 @@ describe('SyntheticEvent', function() {
expect(syntheticEvent.target).toBe(null);
});

it('should warn when accessing 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).toBe(null);
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 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 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() {
spyOn(console, 'error');
var syntheticEvent = createEvent({});
SyntheticEvent.release(syntheticEvent);
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 property `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 +133,10 @@ 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 property `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.'
);
});
});

0 comments on commit 69770b3

Please sign in to comment.