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

Remove use of Proxy for events in development #12171

Closed
gaearon opened this issue Feb 7, 2018 · 9 comments
Closed

Remove use of Proxy for events in development #12171

gaearon opened this issue Feb 7, 2018 · 9 comments

Comments

@gaearon
Copy link
Member

@gaearon gaearon commented Feb 7, 2018

I think maybe we should revert #5947.

People already think proxyEvent is some kind of an API: #12169.

It's also annoying to view in the debugger because none of the properties show up.

Instead, we could seal the event object or something like that.

@koba04
Copy link
Contributor

@koba04 koba04 commented Feb 8, 2018

Object.seal can seal the event object but in a non strict environment, adding extra properties doesn't warn anything, just ignoring it.
It makes debugging so hard.

(In strict mode, we can get an error something like this.)

Uncaught TypeError: Cannot add property foo, object is not extensible

It's a trade off :)

Loading

@gaearon
Copy link
Member Author

@gaearon gaearon commented Feb 8, 2018

I think it's safe to assume most React users run in strict mode by now.

Loading

@jquense
Copy link
Collaborator

@jquense jquense commented Feb 8, 2018

You'd still run into the initial concern in #5853 that Sealing would prevent extension after persist(), granted that's probably not a great pattern but i bet a handful of folks will show up to say some integral abstraction is built on that "feature" :P

Loading

@gaearon
Copy link
Member Author

@gaearon gaearon commented Feb 8, 2018

Can we rewrite the check to just verify the existing fields are not used when we take it from the pool? I just want to get rid of the proxy there. It's annoying.

Loading

@koba04
Copy link
Contributor

@koba04 koba04 commented Feb 9, 2018

I'll work on to verify that the change makes sense.

Loading

@Jessidhia
Copy link
Contributor

@Jessidhia Jessidhia commented Feb 10, 2018

If you are using an ES Proxy and the properties don't show up it sounds like a debugger bug.

Loading

@gaearon
Copy link
Member Author

@gaearon gaearon commented Feb 10, 2018

I don't think so. Trying to enumerate them is potentially side-effectful, it wouldn't be safe for the debugger to do.

Loading

@Jessidhia
Copy link
Contributor

@Jessidhia Jessidhia commented Feb 10, 2018

Only if there is a get or ownKeys trap; but that may be too specific for a debugger 🤔

Loading

@gaearon
Copy link
Member Author

@gaearon gaearon commented Jul 17, 2018

Fixed by #13225.

Loading

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

Successfully merging a pull request may close this issue.

4 participants