-
Notifications
You must be signed in to change notification settings - Fork 48.2k
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.
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
Add a warning for adding a property in the SyntheticEvent object #5947
Add a warning for adding a property in the SyntheticEvent object #5947
Conversation
1b00af6
to
4e6ceb4
Compare
@koba04 updated the pull request. |
should I reimplement this using Proxy? |
Yeah, let's use Proxy objects, since that will allow us to detect writes when they happen, rather than when the event is returned to the pool, which will make the stack trace more useful. |
Thanks!
Ah, that's make sense! |
Ping @koba04 |
Sorry, I'll update this in a few days! |
@koba04 updated the pull request. |
b8b6f33
to
7dd1986
Compare
@koba04 updated the pull request. |
@jimfb I updated this PR! |
var didWarnForAddedNewProperty = false; | ||
var isSupportedProxy = typeof Proxy === 'function'; | ||
|
||
var shouldBeReleasedProperties = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these are fine, just noting this to myself (or others) because it's a potential change in the behavior of this class, but I don't think it has any impact on the overall program behavior.
@koba04 This looks good to me. One minor nitpick (above), plus a rebase is needed to merge. Also, if you could squash these into a single commit ( |
… if Proxy is supported
7dd1986
to
decff26
Compare
apply: function(constructor, that, args) { | ||
return new Proxy(constructor.apply(that, args), { | ||
set: function(target, prop, value) { | ||
if (prop !== 'isPersistent' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this condition is for event.persist()
@koba04 updated the pull request. |
!target.constructor.Interface.hasOwnProperty(prop) && | ||
shouldBeReleasedProperties.indexOf(prop) === -1) { | ||
warning( | ||
didWarnForAddedNewProperty || target.isPersistent(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to avoid a warning in the persistent event.
@jimfb Thank you for your review! I've fixed it. Additionally, I added some changes related persistent event with comments. |
@koba04 updated the pull request. |
1 similar comment
@koba04 updated the pull request. |
Thanks @koba04! |
…-event-property Add a warning for adding a property in the SyntheticEvent object
Thank you!! |
This is a additional work for facebook#5947.
This is a additional work for facebook#5947.
This was a bug introduced by facebook#5947. It's very confusing that they become nulled while stopPropagation/preventDefault don't.
Note for future reference: this was reverted in #13225. |
Fixed #5853