-
-
Notifications
You must be signed in to change notification settings - Fork 32.9k
lib: expose global ErrorEvent #58920
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
Conversation
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #58920 +/- ##
==========================================
- Coverage 90.06% 90.05% -0.01%
==========================================
Files 645 645
Lines 189130 189130
Branches 37094 37093 -1
==========================================
- Hits 170339 170321 -18
- Misses 11511 11516 +5
- Partials 7280 7293 +13
🚀 New features to boost your workflow:
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
cc @nodejs/tsc for
semver-major
|
@KhafraDev Is the class is tested in https://github.com/nodejs/undici inclusive all its related WPTs? Are there ErrorEvent WPTs we could enable in https://github.com/nodejs/node? |
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.
Some form of test coverage should be present here.
It is tested, but all of the WPTs iirc were html files, so I adapted them to run in our own test suite. There isn't anything node needs to enable. It should be safe to assume that, similar to everything else from undici, it follows the spec even closer than pretty much anything in node core. https://github.com/nodejs/undici/blob/main/test/websocket/events.js |
Can you rebase to remove the merge commit please? |
Can we get a re-un of the ci? |
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
Co-authored-by: Antoine du Hamel <[email protected]>
9fa8a55
to
4edeb85
Compare
I think this PR is ready for merge? |
Another day, another try |
Commit Queue failed- Loading data for nodejs/node/pull/58920 ✔ Done loading data for nodejs/node/pull/58920 ----------------------------------- PR info ------------------------------------ Title lib: expose global ErrorEvent (#58920) Author Richie Bendall <[email protected]> (@Richienb) Branch Richienb:expose-errorevent -> nodejs:main Labels semver-major, lib / src, notable-change, author ready, needs-ci, commit-queue-squash Commits 3 - lib: expose global ErrorEvent - Update type-parser.mjs - Update tools/doc/type-parser.mjs Committers 1 - Richie Bendall <[email protected]> PR-URL: https://github.com/nodejs/node/pull/58920 Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Jason Zhang <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/58920 Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Jason Zhang <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 01 Jul 2025 15:34:08 GMT ✔ Approvals: 4 ✔ - Zeyu "Alex" Yang (@himself65): https://github.com/nodejs/node/pull/58920#pullrequestreview-2976453301 ✔ - Matthew Aitken (@KhafraDev): https://github.com/nodejs/node/pull/58920#pullrequestreview-2976691720 ✔ - Jason Zhang (@jazelly): https://github.com/nodejs/node/pull/58920#pullrequestreview-3012989485 ✔ - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/58920#pullrequestreview-2991105194 ✘ semver-major requires at least 2 TSC approvals ✔ Last GitHub CI successful ℹ Last Full PR CI on 2025-08-17T22:17:37Z: https://ci.nodejs.org/job/node-test-pull-request/68686/ - Querying data for job/node-test-pull-request/68686/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/17032906904 |
Landed in 663554a |
PR-URL: nodejs#58920 Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Jason Zhang <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]>
Fixes #58918