X Tutup
The Wayback Machine - https://web.archive.org/web/20220323075347/https://github.com/nodejs/node/pull/37621
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

Jump to bottom

perf_hooks: make Performance extend EventTarget #37621

Closed
wants to merge 1 commit into from
Closed

perf_hooks: make Performance extend EventTarget #37621

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 perf_hooks labels Mar 6, 2021
@targos
Copy link
Member Author

@targos targos commented Mar 6, 2021

Test that passes with this change:

node/test/fixtures/wpt/hr-time/basic.any.js

Lines 30 to 37 in f3d3769

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.");

benjamingr
benjamingr approved these changes Mar 6, 2021
View changes
Copy link
Member

@benjamingr benjamingr left a comment

LGTM

(Is the fact it's an EventTarget actually used by the spec anywhere?)

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 6, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/36504/

@targos
Copy link
Member Author

@targos 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

@benjamingr benjamingr commented Mar 6, 2021

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 label Mar 6, 2021
@targos
Copy link
Member Author

@targos targos commented Mar 6, 2021

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

aduh95
aduh95 approved these changes Mar 6, 2021
View changes
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 6, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/36511/

cjihrig
cjihrig approved these changes Mar 6, 2021
View changes
jasnell
jasnell approved these changes Mar 6, 2021
View changes
Copy link
Member

@jasnell jasnell left a comment

semver-extremely-minor ;-)

It is odd to have an EventTarget with no events but ok.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Mar 8, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/36537/

targos added a commit that referenced this issue Mar 8, 2021
@targos
perf_hooks: make Performance extend EventTarget
5d4fc63 
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 targos commented Mar 8, 2021

Landed in 5d4fc63

@targos targos closed this Mar 8, 2021
@targos targos deleted the performance-event-target branch Mar 8, 2021
@danielleadams
Copy link
Member

@danielleadams danielleadams commented Mar 15, 2021

@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 targos commented Mar 15, 2021

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

@danielleadams
Copy link
Member

@danielleadams danielleadams commented Mar 15, 2021

#37467 landed fine - no need for a backport.

@targos
Copy link
Member Author

@targos targos commented Mar 15, 2021

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

@danielleadams
Copy link
Member

@danielleadams danielleadams commented Mar 16, 2021

@targos v15.x-staging is up to date

@targos targos mentioned this pull request Mar 20, 2021
Closed
targos added a commit to targos/node that referenced this issue Mar 20, 2021
@targos
perf_hooks: make Performance extend EventTarget
688f0bf 
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 added a commit that referenced this issue Mar 29, 2021
@targos @ruyadorno
perf_hooks: make Performance extend EventTarget
4dabb99 
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 added a commit that referenced this issue Mar 30, 2021
@targos @ruyadorno
perf_hooks: make Performance extend EventTarget
e60bd1a 
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
Merged
@targos targos added the dont-land-on-v14.x label Sep 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell

@benjamingr benjamingr

@cjihrig cjihrig

@aduh95 aduh95

@RaisinTen RaisinTen

Assignees
No one assigned
Labels
dont-land-on-v14.x needs-ci perf_hooks semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
9 participants
@targos @nodejs-github-bot @benjamingr @danielleadams @jasnell @cjihrig @aduh95 @RaisinTen @ruyadorno
X Tutup