-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
fix: Expose globalObject
on proxyHandler
[CEReactions] methods
#163
fix: Expose globalObject
on proxyHandler
[CEReactions] methods
#163
Conversation
e29c996
to
246f799
Compare
Hmm, I'm probably missing something obvious, but why does this need to be done here instead of in the jsdom CEReactions processor code? Also, the test snapshots aren't super-helpful in figuring this out, because they just declare a globalObject variable and then don't use it. Could we add a test that uses it, to illustrate when this is helpful? Finally, I'll note that there's an alternate approach for getting per-instance state into proxy handlers, which is to make the proxy handler a class (so that methods are shared in the prototype) and then assign any per-instance state as fields on the class. We'd probably want to measure the performance of such an approach though. |
The webidl2js/test/__snapshots__/test.js.snap Lines 505 to 522 in 246f799
|
Was that an answer to my question of
or just my question about the test snapshots? If so, I don't quite understand. |
After some refactoring, I’ve made it so that there’s now per‑global proxy handlers for interfaces that use This keeps the performance gains from #156 for the most common case (no |
Given that [CEReactions] is on the majority of classes in jsdom, and definitely the majority of performance-sensitive ones, I think we should benchmark such a change before committing to it. |
I ran the performance benchmark in jsdom, and I was able to measure a 5-10% performance regressions when introducing this change on certain benchmarks. The benchmark is comparing the current master of jsdom against the custom elements branch + this PR. The performance improvements can probably be attributed to the create element refactor that has been done on the custom element branch. IMO the performance regression is acceptable compared to value-added by custom element introduction (even if I am completely biased). I am counting a lot on #158 to improve the overall performance even after the introduction of this regression. Results
|
I went to merge this but I'm not sure I can write an accurate commit message. Would the following be an accurate summary of the changes?
I wonder, is |
Since |
Needed for jsdom/jsdom#2548.
See also: #162