events: add once method to use promises with EventEmitter by mcollina · Pull Request #26078 · nodejs/node · GitHub

This change adds a EventEmitter.once() method that wraps ee.once in a
promise. I've been using this model for some months now, and it works extremely well for me.

const { once, EventEmitter } = require('events');
async function run() {
  const ee = new EventEmitter();
  process.nextTick(() => {
    ee.emit('myevent', 42);
  });
  const [value] = await once(ee, 'myevent');
  console.log(value);
  const err = new Error('kaboom');
  process.nextTick(() => {
    ee.emit('error', err);
  });
  try {
    await once(ee, 'myevent');
  } catch (err) {
    console.log('error happened', err);
  }
}
run();

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2592/pipeline

Also @davidmarkclements that had the original idea: would you like to be included as "co-authored-by"?

Why in core? Can't this be an npm module?

Thank you! 💙

Same question as @lpinca -

Why in core? Can't this be an npm module?

I mean, I understand why it would be in core, I think -- that is, to make yet more node.js core apis usable with await.

But I also think that since it doesn't appear to need to be in core, it should at very least have an npm module with adoption to show the weight of it's usefulness...

Fixes #20893 ?

While this could live outside of core, it's been frequently requested and aligns with our promises support. I'm +1 on adding it

Should not this be events.once(emitter, name)? Otherwise, this seems to be a static EventEmitter method like EventEmitter.listenerCount() above. If I understand correctly, this would be called as events.once() in the REPL. I see that it can be used as EventEmitter static method, but in examples, it is imported alongside with the class and this can be confusing for users not aware of EventEmitter.EventEmitter = EventEmitter; core binding.

If I'm not wrong it even works with a third party EventEmitter assuming that it supports once() and removeListener() which kind of reinforces "why in core?". In my opinion this goes against the small core principle.

It is not more efficient than a userland implementation.
It doesn't add something that can't be done in userland.
As far as I know It is not needed in core itself. Is the plan to make core API use it?

I think this API (or something similar) belongs in core for three main reasons:

it aligns with our strategic initiative of “promisifying core APIs”.
it is extremely hard to interact with stream using async/await without an utility like this.
making sure that 12.0.0 has this will shorten the adoption cycle by 1 year.

I think we can use this API in a lot of test code within core.

I’m using it from a userland module, but tiny utility modules are hard to market at this point.

I’m happy to provide more examples if needed.

@lpinca we have been using it for 1 year and a half in Pino test suite (and copy-paste in a lot of other places): https://github.com/pinojs/pino/blob/master/test/basic.test.js#L43 as an example.

Yes, ok but that does not answer "why in core?". A npm module would be better for this in my opinion. Anyway I'm not blocking this and it already has a few TSC approvals.

cc @nodejs/tsc adding to tsc-agenda for visibility

LGTM. I don't disagree with the comment about this possibly belonging in user-land but I'm also happy with it in Node. Implementation is super clean 👍

This is just personal preference but having this at the start of the function body would be clearer for me. When I read the code, the secondListener is referenced before its defined which makes it somewhat confusing. I know it's kind of nitpicky tho so non-blocking...

(Maybe naming it errorRejectListener would also be clearer?)

eventListener and errorListener (or something of the like) would be clearer than main and second.

Related: #20909

No strong opinion, but if there's concerns especially around API design, maybe land as Experimental so we can change the API if we don't get it right the first time?

This change adds a EventEmitter.once() method that wraps ee.once in a promise. Co-authored-by: David Mark Clements <david.mark.clements@gmail.com> PR-URL: nodejs#26078 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Notable changes: * events: * Added a `once` function to use `EventEmitter` with promises (#26078). * tty: * Added a `hasColors` method to `WriteStream` (#26247). * Added NO_COLOR and FORCE_COLOR support (#26485). * v8: * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots in the format used by tools such as Chrome DevTools (#26501). * meta: * Gireesh Punathil is now a member of the Technical Steering Committee (#26657). * Added ZYSzys to collaborators (#26730). PR-URL: #26949

Notable changes: * crypto * Allow deriving public from private keys (Tobias Nießen) [#26278](#26278). * events * Added a `once` function to use `EventEmitter` with promises (Matteo Collina) [#26078](#26078). * tty * Added a `hasColors` method to `WriteStream` (Ruben Bridgewater) [#26247](#26247). * Added NO_COLOR and FORCE_COLOR support (Ruben Bridgewater) [#26485](#26485). * v8 * Added `v8.getHeapSnapshot` and `v8.writeHeapSnapshot` to generate snapshots in the format used by tools such as Chrome DevTools (James M Snell) [#26501](#26501). * worker * Added `worker.moveMessagePortToContext`. This enables using MessagePorts in different vm.Contexts, aiding with the isolation that the vm module seeks to provide (Anna Henningsen) [#26497](#26497). * C++ API * `AddPromiseHook` is now deprecated. This API was added to fill an use case that is served by `async_hooks`, since that has `Promise` support (Anna Henningsen) [#26529](#26529). * Added a `Stop` API to shut down Node.js while it is running (Gireesh Punathil) [#21283](#21283). * meta * [Gireesh Punathil](https://github.com/gireeshpunathil) is now a member of the Technical Steering Committee [#26657](#26657). * Added [Yongsheng Zhang](https://github.com/ZYSzys) to collaborators [#26730](#26730). PR-URL: #26949

This change adds a EventEmitter.once() method that wraps ee.once in a promise. Co-authored-by: David Mark Clements <david.mark.clements@gmail.com> PR-URL: #26078 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Why not do the same for on?
Then it would be possible to have a bus based on promises

.once() callbacks once, promises resolve once. .on() callbacks repeatedly, so how could that work with promises, which cannot resolve multiple times?

Why not do the same for on?

We could expose a on(ee, 'event') method that returns an async iterator. However there would be no "exit" condition from the loop.

for await (let event of on(ee, 'event')) {
  // do something with event
  // this loop will never end
}

Would you mind opening a new issue to discuss? Do you think this would be worthwhile even if the loop would never end?

Well, problem I have so far, if I have an active subscription to events that can come forever until I unsubscribe, there is no way to have a nice handling of that with promises.
Basically I am looking for a "hot observable" kind of thing.

I would recommend using a stream instead. It’s async iterable already.

We could support an on function that returns an async iterator so long as we introduced a concept of a loop terminator.... That is, an object that would cause the loop to terminate...

E.g. something roughly like...

const foo = events.on(emitter, 'foo', term);
for await (const f of foo) {
  // ...
  term.exit()
}

@jasnell @mcollina actually break and return would work:

const foo = events.on(emitter, 'foo');
for await (const f of foo) {
  break; // this calls `.return` on the async iterable and can unsubscribe.
}

This is also true for regular generators. I'm +1 on exploring this.

@benjamingr ... yes, those work within the actual for await but we would need to make sure we're able to clean up inside the async iterator itself (e.g. removing the event listener). It may be rather trivial to do, I just haven't tried it yet :-) ... Perhaps I'll take a break from QUIC implementation and give it a try :-)

Ok, quick investigation... it should be possible to implement an EventEmitter.on(emitter, event) function that is essentially a simplification of the code used for stream.Readable with a few notable differences:

Unlike stream.Readable, there is no natural end to the event stream, and there is no destroy() method, so cleanup and termination of the for await() {} would be entirely dependent on using break and return as suggested.
With stream.Readable, when there are multiple synchronous push()'s, there's still just a single 'readable', and just in case there are multiple read() operations performed. For an arbitrary EventEmitter, we would need a way of reliably handling synchronous emits.

Everything else looks like it should be fairly reasonable to implement if someone wants to take it on ;-)

moving to a separate issue

Notable changes: - deps: - icu 63.1 bump (CLDR 34) (Steven R. Loomis) [#23715](#23715) - upgrade npm to 6.9.0 (Kat Marchán) [#26244](#26244) - upgrade openssl sources to 1.1.1a (Sam Roberts) [#25381](#25381) - upgrade to libuv 1.24.1 (cjihrig) [#25078](#25078) - events: add once method to use promises with EventEmitter (Matteo Collina) [#26078](#26078) - n-api: mark thread-safe function as stable (Gabriel Schulhof) [#25556](#25556) - repl: support top-level for-await-of (Shelley Vohr) [#23841](#23841) - zlib: - add brotli support (Anna Henningsen) [#24938](#24938) PR-URL: #27514

Notable changes: - **deps**: - update ICU to 64.2 (Ujjwal Sharma) [#27361](#27361) - upgrade npm to 6.9.0 (Kat Marchán) [#26244](#26244) - upgrade openssl sources to 1.1.1b (Sam Roberts) [#26327](#26327) - upgrade to libuv 1.28.0 (cjihrig) [#27241](#27241) - **events**: - add once method to use promises with EventEmitter (Matteo Collina) [#26078](#26078) - **n-api**: - mark thread-safe function as stable (Gabriel Schulhof) [#25556](#25556) - **repl**: - support top-level for-await-of (Shelley Vohr) [#23841](#23841) - **zlib**: - add brotli support (Anna Henningsen) [#24938](#24938) PR-URL: #27514

mcollina requested review from apapirovski and jasnell February 13, 2019 14:50

nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 13, 2019

jasnell approved these changes Feb 13, 2019

View reviewed changes

BridgeAR approved these changes Feb 13, 2019

View reviewed changes

mcollina added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 13, 2019

devsnek approved these changes Feb 13, 2019

View reviewed changes

jasnell reviewed Feb 13, 2019

View reviewed changes

Comment thread doc/api/events.md Outdated

jasnell reviewed Feb 13, 2019

View reviewed changes

Comment thread doc/api/events.md Outdated

addaleax approved these changes Feb 13, 2019

View reviewed changes

Fishrock123 reviewed Feb 13, 2019

View reviewed changes

vsemozhetbyt reviewed Feb 14, 2019

View reviewed changes

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 14, 2019

targos approved these changes Feb 14, 2019

View reviewed changes

mhdawson mentioned this pull request Feb 18, 2019

Node.js Foundation Technical Steering Committee (TSC) Meeting 2019-02-20 nodejs/TSC#668

Closed

apapirovski approved these changes Feb 20, 2019

View reviewed changes

ChALkeR self-requested a review February 20, 2019 21:19

Trott removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Feb 20, 2019

targos mentioned this pull request Mar 27, 2019

v11.13.0 release proposal #26949

Merged

BethGriggs mentioned this pull request May 1, 2019

v10.16.0 proposal #27514

Merged

benjamingr mentioned this pull request May 24, 2019

Async Iterable EventEmitter.on(emitter, "event") #27847

Closed

leumasme mentioned this pull request Jan 16, 2022

Promise-based events.once in typed-emitter andywer/typed-emitter#24

Open

events: add once method to use promises with EventEmitter by mcollina · Pull Request #26078 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: add once method to use promises with EventEmitter - #26078

events: add once method to use promises with EventEmitter#26078mcollina wants to merge 1 commit intonodejs:masternodejs/node:masterfrom mcollina:oncemcollina/node:onceCopy head branch name to clipboard

Conversation

mcollina commented Feb 13, 2019

Checklist

Uh oh!

nodejs-github-bot commented Feb 13, 2019

Uh oh!

mcollina commented Feb 13, 2019

Uh oh!

Uh oh!

Uh oh!

lpinca commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Feb 13, 2019

Uh oh!

jasnell commented Feb 13, 2019

Uh oh!

vsemozhetbyt Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Feb 14, 2019

Uh oh!

mcollina commented Feb 14, 2019

Uh oh!

lpinca commented Feb 14, 2019

Uh oh!

mcollina commented Feb 14, 2019

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

apapirovski Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Feb 20, 2019

Uh oh!

Trott commented Feb 20, 2019

Uh oh!

tonnyavery commented May 22, 2019

Uh oh!

sam-github commented May 22, 2019

Uh oh!

mcollina commented May 22, 2019

Uh oh!

tonnyavery commented May 22, 2019

Uh oh!

mcollina commented May 22, 2019

Uh oh!

jasnell commented May 22, 2019

Uh oh!

benjamingr commented May 22, 2019

Uh oh!

jasnell commented May 22, 2019

Uh oh!

jasnell commented May 23, 2019

Uh oh!

benjamingr commented May 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

events: add once method to use promises with EventEmitter#26078
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:once

lpinca commented Feb 13, 2019 •
edited
Loading

Fishrock123 left a comment •
edited
Loading

vsemozhetbyt Feb 14, 2019 •
edited
Loading

lpinca commented Feb 14, 2019 •
edited
Loading

apapirovski Feb 20, 2019 •
edited
Loading

benjamingr commented May 24, 2019 •
edited
Loading