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

perf_hooks: make Performance extend EventTarget #37621

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Mar 6, 2021

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. labels Mar 6, 2021
@targos
Copy link
Member Author

targos commented Mar 6, 2021

Test that passes with this change:

test(function() {
var didHandle = false;
self.performance.addEventListener("testEvent", function() {
didHandle = true;
}, { once: true} );
self.performance.dispatchEvent(new Event("testEvent"));
assert_true(didHandle, "Performance extends EventTarget, so event dispatching should work.");
}, "Performance interface extends EventTarget.");

Copy link
Member

@benjamingr benjamingr left a 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?)

@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member Author

targos commented Mar 6, 2021

@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 basic.any.js.

@benjamingr
Copy link
Member

I don't know if it's important for us to extend EventTarget.

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 :)

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 6, 2021
@targos
Copy link
Member Author

targos commented Mar 6, 2021

I suppose it's a (very minor 😄) semver-minor change.

@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@jasnell jasnell left a 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.

@nodejs-github-bot
Copy link
Collaborator

targos added a commit that referenced this pull request Mar 8, 2021
Refs: https://www.w3.org/TR/hr-time/#sec-performance

PR-URL: #37621
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
Copy link
Member Author

targos commented Mar 8, 2021

Landed in 5d4fc63

@targos targos closed this Mar 8, 2021
@targos targos deleted the performance-event-target branch March 8, 2021 19:35
@danielleadams
Copy link
Contributor

@targos can you create a backport PR for this for v15.x-staging? It doesn't land cleanly because #37136 is a semver major change. Thank you!

@targos
Copy link
Member Author

targos commented Mar 15, 2021

#37467 should land first. Does it also need to be backported?

@danielleadams
Copy link
Contributor

#37467 landed fine - no need for a backport.

@targos
Copy link
Member Author

targos commented Mar 15, 2021

Okay, I'll just wait for v15.x-staging to be pushed and I'll backport.

@danielleadams
Copy link
Contributor

@targos v15.x-staging is up to date

targos added a commit to targos/node that referenced this pull request Mar 20, 2021
Refs: https://www.w3.org/TR/hr-time/#sec-performance

PR-URL: nodejs#37621
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Mar 29, 2021
Refs: https://www.w3.org/TR/hr-time/#sec-performance

PR-URL: #37621
Backport-PR-URL: #37832
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Mar 30, 2021
Refs: https://www.w3.org/TR/hr-time/#sec-performance

PR-URL: #37621
Backport-PR-URL: #37832
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. perf_hooks Issues and PRs related to the implementation of the Performance Timing API. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants