Skip to content

Conversation

Renegade334
Copy link
Contributor

@Renegade334 Renegade334 commented Oct 11, 2025

Resolves #60212.

Makes events.listenerCount() agnostic over both EventEmitters and EventTargets.

For EventTargets, this is faster than the events.getEventListeners(...).length pattern currently in use within the test suite, as it doesn't need to traverse the linked list.

Note that this now validates the emitter argument, whereas the previous version did not (it just returned 0 if the target wasn't an EventEmitter). Dealer's choice as to whether or not this constitutes a minor or major change.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@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 Oct 11, 2025
@Renegade334 Renegade334 force-pushed the events-listenercount branch 5 times, most recently from 4a748e4 to 336f0e5 Compare October 11, 2025 21:20
@Renegade334 Renegade334 marked this pull request as ready for review October 11, 2025 22:41
@@ -59,7 +59,8 @@ Stream.prototype.pipe = function(dest, options) {
// Don't leave dangling pipes when there are errors.
function onerror(er) {
cleanup();
if (EE.listenerCount(this, 'error') === 0) {
// If we removed the last error handler, trigger an unhandled error event.
if (this.listenerCount?.('error') === 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can't use the new validated version of events.listenerCount(). Legacy behaviour dictates that stream.pipe() targets do not need to be true EventEmitters; this.listenerCount might be a user-defined function, or might not exist at all (#2655).

Copy link

codecov bot commented Oct 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.54%. Comparing base (367bcce) to head (f723962).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60214      +/-   ##
==========================================
- Coverage   88.55%   88.54%   -0.01%     
==========================================
  Files         704      704              
  Lines      208087   208128      +41     
  Branches    40017    40008       -9     
==========================================
+ Hits       184267   184286      +19     
- Misses      15817    15867      +50     
+ Partials     8003     7975      -28     
Files with missing lines Coverage Δ
lib/events.js 99.83% <100.00%> (+<0.01%) ⬆️
lib/internal/streams/legacy.js 94.44% <100.00%> (+0.04%) ⬆️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

Suggestion: Re-purpose events.listenerCount() as agnostic function over EventEmitters/EventTargets

2 participants