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: add once method to use promises with EventEmitter #26078

Closed
wants to merge 1 commit into from

Conversation

@mcollina
Copy link
Member

commented Feb 13, 2019

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 mcollina requested review from apapirovski and jasnell Feb 13, 2019

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

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

Show resolved Hide resolved doc/api/events.md Outdated
Show resolved Hide resolved doc/api/events.md Outdated
@lpinca

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

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

@addaleax
Copy link
Member

left a comment

Thank you! 💙

@Fishrock123
Copy link
Member

left a comment

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

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Fixes #20893 ?

@jasnell

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

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

@@ -653,6 +653,46 @@ newListeners[0]();
emitter.emit('log');
```

## EventEmitter.once(emitter, name)

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Feb 14, 2019

Member

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.

@lpinca

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

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?
@mcollina

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

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

  1. it aligns with our strategic initiative of “promisifying core APIs”.
  2. it is extremely hard to interact with stream using async/await without an utility like this.
  3. 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.

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

@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.

@lpinca

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

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.

@mcollina mcollina added the tsc-agenda label Feb 14, 2019

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

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

@targos

targos approved these changes Feb 14, 2019

@apapirovski
Copy link
Member

left a comment

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 👍

}
resolve(args);
};
let secondListener;

This comment has been minimized.

Copy link
@apapirovski

apapirovski Feb 20, 2019

Member

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?)

This comment has been minimized.

Copy link
@sam-github

sam-github Feb 21, 2019

Member

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

@ChALkeR ChALkeR self-requested a review Feb 20, 2019

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Related: #20909

@Trott

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

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?

@Trott Trott removed the tsc-agenda label Feb 20, 2019

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

@ChALkeR PTAL

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

@ChALkeR

ChALkeR approved these changes Mar 2, 2019

Copy link
Member

left a comment

Code LGTM.
Let's wait for CI, but overall LGTM if CI passes.
Nice work! 🎉

@ChALkeR ChALkeR added the author ready label Mar 2, 2019

@mcollina mcollina force-pushed the mcollina:once branch from 32e096d to de02e3e Mar 2, 2019

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

A testcase for ee.emit('myevent', 42, 2); ee.emit('error', err); could be nice as an optional addition, but I'm not going to block on that. Restored the label. 😃

Upd: ah, I see the new listenerCount() checks, they should cover that, thanks! 🎉

events: add once method to use promises with EventEmitter
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>
@mcollina

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

I've added a tiny edit to the doc.

@mcollina mcollina force-pushed the mcollina:once branch from de02e3e to 30b99fe Mar 2, 2019

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

CI is ok on Jenkins, landing. I'm not sure why here on GH it reports failure

@mcollina

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

Landed in b1ef279

@mcollina mcollina closed this Mar 2, 2019

@mcollina mcollina deleted the mcollina:once branch Mar 2, 2019

mcollina added a commit that referenced this pull request Mar 2, 2019

events: add once method to use promises with EventEmitter
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>
@ChALkeR

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

@mcollina The failing status on GH was from https://ci.nodejs.org/job/node-test-pull-request/21118/, not https://ci.nodejs.org/job/node-test-pull-request/21119/ for whatever reason.

Upd: GH didn't linkify the commit id above for me, so here is a link: b1ef279.

@davidmarkclements

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

@vsemozhetbyt vsemozhetbyt referenced this pull request Mar 6, 2019

Closed

doc: add caveat and tradeoff example to readline #26472

3 of 3 tasks complete

targos added a commit to targos/node that referenced this pull request Mar 27, 2019

events: add once method to use promises with EventEmitter
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>

targos added a commit that referenced this pull request Mar 27, 2019

2019-03-28, Version 11.13.0 (Current)
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).

targos added a commit that referenced this pull request Mar 27, 2019

2019-03-28, Version 11.13.0 (Current)
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

@targos targos referenced this pull request Mar 27, 2019

Merged

v11.13.0 release proposal #26949

targos added a commit that referenced this pull request Mar 28, 2019

2019-03-28, Version 11.13.0 (Current)
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

targos added a commit that referenced this pull request Mar 28, 2019

2019-03-28, Version 11.13.0 (Current)
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

BethGriggs added a commit that referenced this pull request Apr 5, 2019

2019-03-28, Version 11.13.0 (Current)
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

BethGriggs added a commit that referenced this pull request Apr 16, 2019

events: add once method to use promises with EventEmitter
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.