events: add addDisposableListener method to EventEmitter

[jasnell]

This adds new use(...) and useOnce(...) methods addDisposableListener method to EventEmitter that returns a disposable object that will unregister the event listeners when disposed, along with two changes that use the new apis to demonstrate how it can be used to simplify some cleanup.

const ee = new EventEmitter();
{
  using ds = new DisposableStack();
  ds.use(ee.addDisposableListener('foo', () => {});
  ds.use(ee.addDisposableListener('bar', () => {});
  if (something) ds.use(ee.addDisposableListener('baz', () => {});
}
// The event listeners will all be removed automatically when ds is disposed.
// We don't need to separately remember to call removeListener/off to cleanup the event listeners

[nodejs-github-bot]

Review requested:

• @nodejs/streams
• @nodejs/test_runner

[jasnell]

Relevant to advancing on this: #58526

[mcollina]

I'm ok with the addition. I'm a bit worried about the potential change in behavior, as I didn't have time to explore yet if there is a change in the order of operations.

[jasnell]

What change in behavior are you concerned about? This should not change any existing behavior of the EventEmitter

[jasnell]

I've updated the implementation to rename the use method to a single addDisposableListener method with a once option. It returns a dispose function that has the Symbol.dispose property attached to call itself. PTAL

[LiviaMedeiros]

LGTM with removal of obsolete no-undefs and assuming consensus on this in #58526.
Perhaps using dispose === dispose[Symbol.dispose] can be made into recommended pattern (whenever we don't have more meaningful return values) in the ERM guideline.

[benjamingr]

Is there any way we can avoid adding yet another way to register events? i.e. can this be an option and not a method?

I think this was blocked previously in AbortSignal due to a V8 optimization that happened since.

[jasnell]

Is there any way we can avoid adding yet another way to register events? i.e. can this be an option and not a method?

Not without adding too much complexity. As it is now, addListener(...) returns the EventEmitter itself. Adding an option would mean making the return value polymorphic which is far worse than adding a new separate API. See the discussion around #58526

[Renegade334]

Regarding the single-use disposable that's just a disposer, with no associated object data: is

function dispose() { ... }
dispose[SymbolDispose] = dispose
return dispose

OK, or should we be keeping things consistent with the draft guidance for disposers in general?

function dispose() { ... }
return {
  dispose,
  [SymbolDispose]: dispose,
  // or
  [SymbolDispose]() { dispose() },
}

My vote would be for the latter – it would be an inconsistent pattern for disposables to sometimes be directly callable, and other times not.

[nodejs-github-bot]

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

[codecov]

Codecov Report

Attention: Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 90.14%. Comparing base (3c351c2) to head (f4ca9ae).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/streams/end-of-stream.js	94.11%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58453      +/-   ##
==========================================
+ Coverage   90.13%   90.14%   +0.01%     
==========================================
  Files         636      636              
  Lines      187891   187932      +41     
  Branches    36878    36883       +5     
==========================================
+ Hits       169348   169408      +60     
- Misses      11289    11290       +1     
+ Partials     7254     7234      -20     

Files with missing lines	Coverage Δ
lib/events.js	99.60% <100.00%> (+0.01%)	⬆️
lib/internal/test_runner/harness.js	93.41% <100.00%> (+0.06%)	⬆️
lib/internal/streams/end-of-stream.js	97.23% <94.11%> (-0.05%)	⬇️

... and 34 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.

[legendecas]

Returning an anonymous object in the API makes it hard to discover, like searching API interfaces with [Symbol.dispose] method. Additionally, the properties of the anynomous objects are not as clear as a named types. I'd prefer a named interface for the event listener disposable type.