Skip to content

Commit

Permalink
Remove the use of proxies for synthetic events in DEV (#13225)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@gaearon
gaearon authored Jul 17, 2018
1 parent 171e0b7 commit acbb4f9
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 71 deletions.
84 changes: 26 additions & 58 deletions packages/events/SyntheticEvent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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/
Expand Down Expand Up @@ -81,6 +70,8 @@ function SyntheticEvent(
delete this.nativeEvent;
delete this.preventDefault;
delete this.stopPropagation;
delete this.isDefaultPrevented;
delete this.isPropagationStopped;
}

this.dispatchConfig = dispatchConfig;
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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);

/**
Expand Down Expand Up @@ -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) {
Expand Down
82 changes: 69 additions & 13 deletions packages/react-dom/src/events/__tests__/SyntheticEvent-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(<div onClick={eventHandler} />, 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(<div onClick={eventHandler} />, 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) {
Expand All @@ -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++;
};
Expand Down

0 comments on commit acbb4f9

Please sign in to comment.