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.

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

Seal pooled events? #5853

Closed
syranide opened this issue Jan 14, 2016 · 11 comments
Closed

Seal pooled events? #5853

syranide opened this issue Jan 14, 2016 · 11 comments

Comments

@syranide
Copy link
Contributor

Kind of related to the old #1612 and #5840, anyone trying to be smart and adding properties to a pooled non-persisted event will cause those to properties to remain in the pool, causing a memory leak of sorts. Could it make sense to Object.seal(event) the events, that way we ensure that users can't mess with the event object in bad or unsupported ways.

Should probably do this instead: #5853 (comment)

@jimfb
Copy link
Contributor

jimfb commented Jan 14, 2016

But you can't unseal when the user does persist the object, right? You're suggesting that users are never allowed to add properties to synthetic events?

@syranide
Copy link
Contributor Author

@jimfb Yeah, that's obviously an issue. I would call it bad practice to modify events like that, so I personally wouldn't mind preventing it, but it might still be something we want to support. We could also clone instead of persisting events (same number of allocations), but it would obviously affect the API (event = event.clone()).

@jimfb
Copy link
Contributor

jimfb commented Jan 14, 2016

Would it be sufficient to just detect the problem and warn?

We could check to verify that there are no extraneous properties on the event object after the event handler has completed, right? Or we could wrap the event in a proxy object (in dev mode and where supported - firefox, edge, chrome-canary, etc) and warn if someone mutates an event before persisting it.

@syranide
Copy link
Contributor Author

Would it be sufficient to just detect the problem and warn? We could check to verify that there are no extraneous properties on the event object after the event handler has completed, right?

Yeah that should be fine too. Ultimately you're messing up the pooled events so it really should be an error. But then it couldn't be DEV-only and that's bad.

Or we could wrap the event in a proxy object (in dev mode and where supported - firefox, edge, chrome-canary, etc) and warn if someone mutates an event before persisting it.

Tangentially, we could even use this to warn when accessing an event after it has been returned to the pool?

@jimfb
Copy link
Contributor

jimfb commented Jan 14, 2016

Yeah that should be fine too. Ultimately you're messing up the pooled events so it really should be an error. But then it couldn't be DEV-only and that's bad.

There are lots of things that I believe should be errors, but the javascript community seems to like things that fail soft. That was one of my biggest culture shocks, coming from a java background where the community believes that things should always fail super hard. I've given up on that fight, need to choose my battles :P.

Tangentially, we could even use this to warn when accessing an event after it has been returned to the pool?

We could do that anyway, with Object.defineProperty, right? Since we know what properties exist on events.

Anyway, overall, I agree this seems like a useful feature to implement 👍

@syranide
Copy link
Contributor Author

There are lots of things that I believe should be errors, but the javascript community seems to like things that fail soft. That was one of my biggest culture shocks, coming from a java background where the community believes that things should always fail super hard. I've given up on that fight, need to choose my battles :P.

I agree, but this is more because we don't want the perf hit in production I would say.

We could do that anyway, with Object.defineProperty, right? Since we know what properties exist on events.

Yeah I guess so... but it's probably a little involved and it technically changes the runtime behavior (they are now getters/setters instead of values), I doubt anyone would notice but still. IMHO it's fine if DEV-warnings for coding practices only work in modern browsers.

PS. Hmm, didn't we have something like this? Perhaps we already do... I remember @spicyj working on something like it, but perhaps it never shipped.

@syranide
Copy link
Contributor Author

@jimfb Good first "bug"?

@jimfb
Copy link
Contributor

jimfb commented Jan 14, 2016

Yep.

@koba04
Copy link
Contributor

koba04 commented Jan 14, 2016

I'll work on this after #5840.

@antsmartian
Copy link
Contributor

@jimfb Hi, I was trying to work on this issue. As you have mentioned:

Would it be sufficient to just detect the problem and warn? We could check to verify that there are no extraneous properties on the event object after the event handler has completed, right?

I tried to figure out are there any "hooks" for finding when the event handler have been completed. While I navigated the React source I found that ReactEventListener has a method called dispatchEvent which is what firing the event and its associated call backs. However I couldn't able to make out, when the Synthetic events are getting created and also when the event handler execution is complete.

Any help?

@koba04
Copy link
Contributor

koba04 commented Feb 2, 2016

I made a PR for this #5947.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants