-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Contextify should use ES6 Proxy to wrap the global object. #7820
Comments
cc @bnoordhuis |
I can have a look at this |
Please note the tracking issue listing all the known bugs w/ contextify here: #6283. @verwaest, @fhinkel, @domenic, @jeisinger and I had a discussion on this a while back and came to the conclusion that the only real solution to the problem is going to be if the global object in the new context is physically the same object in the outer context for things like IMO, the real problem stems from the fact that interceptors don't trigger for all events. A new API in V8 that would allow us to intercept these would be the way to fix this problem. |
A Proxy would definitely help with some of these issues at the cost of making every operation run inside the vm slower. Using dedicated V8 APIs as discussed previously seems like a better path. |
So performance is a real concern here? Do you have any benchmarks? I do agree that at this point, Proxies are slow. But I personally would prefer making Proxies faster, for everyone, instead of adding a special hook for this particular use case. One that can be solved by using a Proxy for its designed purpose. Like Ali said, interceptors don't trigger for all events. Is there any other event other than Object.defineProperty that does not trigger? |
Some offline discussions and knowing more about the requirements, I now see that Proxies are indeed not a valid solution. We do want to keep the sandbox object and the context's global object separate. |
@ofrobots @fhinkel @verwaest @isaacs
I'm recently browsing through node_contextify.cc and found this comment describing ContextifyContext::CopyProperties:
So let me see if I understood this correctly:
Seems like wrapping the global object in an ES6 Proxy would solve the issue. I wonder whether doing that is straight-forward. We could then also get rid of the named property interceptors and the hidden prototype.
The text was updated successfully, but these errors were encountered: