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

events: fix undefined passes to arrayClone #56264

Closed
wants to merge 3 commits into from

Conversation

origranot
Copy link
Contributor

@origranot origranot commented Dec 15, 2024

Fixes #56263

Another suggestion is to force developers to pass a string, else throw an ERR_INVALID_ARG_TYPE but this should consider a breaking change.

Looking forward for your feedback!

@nodejs-github-bot nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. labels Dec 15, 2024
@anonrig
Copy link
Member

anonrig commented Dec 15, 2024

Please add a test case

@origranot
Copy link
Contributor Author

Please add a test case

Done 💯

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (a50f3d5) to head (6b93137).
Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #56264      +/-   ##
==========================================
- Coverage   88.55%   88.54%   -0.01%     
==========================================
  Files         657      657              
  Lines      190243   190245       +2     
  Branches    36536    36542       +6     
==========================================
- Hits       168461   168457       -4     
- Misses      14963    14968       +5     
- Partials     6819     6820       +1     
Files with missing lines Coverage Δ
lib/events.js 99.83% <100.00%> (+<0.01%) ⬆️

... and 25 files with indirect coverage changes

@lpinca
Copy link
Member

lpinca commented Dec 16, 2024

This patch makes me wonder if 7a461ed was a good idea. I find it harder to read it now. I would revert 7a461ed and add the test included here.

Copy link
Member

@jakecastelli jakecastelli left a comment

Choose a reason for hiding this comment

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

I am actually open to (prefer to) revert and add a test as well

Copy link
Member

@jazelly jazelly left a comment

Choose a reason for hiding this comment

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

Had a private chat with @jakecastelli. I think it's better to revert this and add a test.

@origranot Would you like to raise a new PR to revert the commit and introduce a new commit to add the test?

@origranot
Copy link
Contributor Author

Had a private chat with @jakecastelli. I think it's better to revert this and add a test.

@origranot Would you like to raise a new PR to revert the commit and introduce a new commit to add the test?

Yea, I am on it.

Thanks!

@origranot
Copy link
Contributor Author

origranot commented Dec 17, 2024

@jazelly
I opened a new PR here: #56282

@origranot origranot requested a review from jazelly December 17, 2024 08:30
@lpinca
Copy link
Member

lpinca commented Dec 19, 2024

Closed in favor of #56282.

@lpinca lpinca closed this Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EventEmitter] TypeError when calling .listeners() without arguments
7 participants