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.

Sign up for GitHub

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

events: allow null/undefined eventInitDict #54643

Closed
wants to merge 2 commits into from
Closed

events: allow null/undefined eventInitDict #54643

wants to merge 2 commits into from
+12 −6

Conversation

KhafraDev
Copy link
Member

No description provided.

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 29, 2024
@KhafraDev KhafraDev added the eventtarget Issues and PRs related to the EventTarget implementation. label Aug 29, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 29, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.30%. Comparing base (ff5ef70) to head (3d83fe5).
Report is 280 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54643      +/-   ##
==========================================
- Coverage   87.34%   87.30%   -0.05%     
==========================================
  Files         649      649              
  Lines      182706   182755      +49     
  Branches    35046    35052       +6     
==========================================
- Hits       159590   159558      -32     
- Misses      16388    16464      +76     
- Partials     6728     6733       +5
Files with missing lines Coverage Δ
lib/internal/event_target.js 98.28% <100.00%> (ø)

... and 29 files with indirect coverage changes

@RedYetiDev
Copy link
Member

This feature changes an existing behavior (shown below):

$ node -e "new Event('', null);"
node:internal/errors:540
      throw error;
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received null
    at new Event (node:internal/event_target:117:5)
    at [eval]:1:1
    at runScriptInThisContext (node:internal/vm:209:10)
    at node:internal/process/execution:118:14
    at [eval]-wrapper:6:24
    at runScript (node:internal/process/execution:101:62)
    at evalScript (node:internal/process/execution:136:3)
    at node:internal/main/eval_string:55:3 {
  code: 'ERR_INVALID_ARG_TYPE'
}

Node.js v22.7.0

Is it semver-major?

@KhafraDev
Copy link
Member Author

this is a bug fix

@RedYetiDev
Copy link
Member

this is a bug fix

ahh okay, thanks for the clarification.

@KhafraDev KhafraDev added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@KhafraDev
Copy link
Member Author

the ci is killing me

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
jazelly
jazelly reviewed Sep 4, 2024
View reviewed changes
lib/internal/event_target.js
@@ -111,14 +111,14 @@ class Event {
* composed?: boolean,
* }} [options]
*/
constructor(type, options = kEmptyObject) {
constructor(type, options = undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also update jsdoc?

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Sep 8, 2024

PR is currently blocked from landing by unreliable CI.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 19, 2024 •
edited by jasnell
Loading

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

jasnell pushed a commit that referenced this pull request Sep 21, 2024
@KhafraDev @jasnell
events: allow null/undefined eventInitDict
a9081b5 
PR-URL: #54643
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 21, 2024

Landed in a9081b5

@jasnell jasnell closed this Sep 21, 2024
@KhafraDev KhafraDev deleted the null-undefined-eventinitdict branch September 21, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jazelly jazelly jazelly left review comments

@jasnell jasnell jasnell approved these changes

@anonrig anonrig anonrig approved these changes

@JungMinu JungMinu JungMinu approved these changes

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. eventtarget Issues and PRs related to the EventTarget implementation. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@KhafraDev @RedYetiDev @nodejs-github-bot @jasnell @anonrig @JungMinu @jazelly @LiviaMedeiros