-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
perf_hooks: make Performance extend EventTarget #37621
Conversation
Test that passes with this change: node/test/fixtures/wpt/hr-time/basic.any.js Lines 30 to 37 in f3d3769
|
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.
LGTM
(Is the fact it's an EventTarget actually used by the spec anywhere?)
@benjamingr It's used by another spec that extends Performance: https://developer.mozilla.org/en-US/docs/Web/API/Performance/resourcetimingbufferfull_event I don't know if it's important for us to extend EventTarget. I mostly implemented this so we can stop skipping |
I think it's not very important - I think it's beneficial regardless since it aligns us with the spec and it lets developers write universal code more easily without actually increasing maintenance cost :) |
I suppose it's a (very minor 😄) semver-minor change. |
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.
semver-extremely-minor
;-)
It is odd to have an EventTarget
with no events but ok.
Refs: https://www.w3.org/TR/hr-time/#sec-performance PR-URL: #37621 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Landed in 5d4fc63 |
#37467 should land first. Does it also need to be backported? |
#37467 landed fine - no need for a backport. |
Okay, I'll just wait for v15.x-staging to be pushed and I'll backport. |
@targos |
Refs: https://www.w3.org/TR/hr-time/#sec-performance PR-URL: nodejs#37621 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: https://www.w3.org/TR/hr-time/#sec-performance PR-URL: #37621 Backport-PR-URL: #37832 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: https://www.w3.org/TR/hr-time/#sec-performance PR-URL: #37621 Backport-PR-URL: #37832 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Refs: https://www.w3.org/TR/hr-time/#sec-performance