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 EventEmitter.on to async iterate over events #27994

Closed
wants to merge 17 commits into from

Conversation

@mcollina
Copy link
Member

mcollina commented May 31, 2019

Fixes: #27847.

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 jasnell, benjamingr, devsnek and BridgeAR May 31, 2019
},

throw(err) {
// TODO should we validate that err exists?

This comment has been minimized.

Copy link
@benjamingr

benjamingr May 31, 2019

Member

No, I think we shouldn't since it is allowed to throw undefined. I really hope users don't actually do it.

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

This comment has been minimized.

Copy link
Member

benjamingr commented May 31, 2019

CC @davidmarkclements since you maintain the .on shim in case you want to make a shim for this on NPM. If you don't I don't mind making one.

@mcollina mcollina force-pushed the mcollina:on-async-iterator branch 2 times, most recently from 6e69e8d to b6542f0 May 31, 2019
@zero1five

This comment has been minimized.

Copy link
Contributor

zero1five commented Jun 1, 2019

I like this!! 😆
I studied the this API for five or six hours, and there's something odd about it that I want to point out (if I stray from the subject, feel free to hide).

One test case seems to have a bug. I'm not sure what caused it Gist code

Specific steps for recurrence:

git pr 27994 upstream
./configure && make -j4
../Release/node ../index.js

It is clear that the async function exits directly after the for await of.

@davidmarkclements

This comment has been minimized.

Copy link
Member

davidmarkclements commented Jun 1, 2019

CC @davidmarkclements since you maintain the .on shim in case you want to make a shim for this on NPM. If you don't I don't mind making one.

thanks @benjamingr - I've had the events.on shim ready since releasing events.once 😆 - I'm just waiting for core to land in case I need to adapt

@jasnell
jasnell approved these changes Jun 1, 2019
@devsnek
devsnek approved these changes Jun 1, 2019
Copy link
Contributor

vsemozhetbyt left a comment

Thank you!

@@ -694,6 +694,37 @@ async function run() {
run();
```

## events.on(emitter, event)

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Jun 1, 2019

Contributor

Nit: needs to be placed before events.once() alphabetically?

doc/api/events.md Show resolved Hide resolved
@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Jun 2, 2019

@zero1five thanks for the test case, definitely relevant I'll look into it when I'm in front of a computer

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Jun 2, 2019

@zero1five hey, I ran your code example but I wasn't able to figure out what the confusing part was. Do you mean how that pending promises do not keep the event loop live if there is no scheduled I/O?

@addaleax addaleax added the events label Jun 2, 2019
@Raynos

This comment has been minimized.

Copy link
Contributor

Raynos commented Jun 7, 2019

Some events in node core emit a single value.

Would it be possible to add an option to this api to return the first value emitted instead of an array of all values emitted.

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Jun 7, 2019

@Raynos hey, long time no see. Happy to see you around here :]

Node already shipped (recently) an EventEmitter.once static method that does just that. https://nodejs.org/api/events.html#events_events_once_emitter_name

@Raynos

This comment has been minimized.

Copy link
Contributor

Raynos commented Jun 7, 2019

@Raynos hey, long time no see. Happy to see you around here :]

I was at the openjs summit in berlin, next time I see you i'll say hello properly.

Node already shipped (recently) an EventEmitter.once static method that does just that.

I meant an option to return the first value emitted, aka emitter.emit('foo', firstValue) instead of only listening to a single emission of the event like once.

The current implementation would require some destructuring in the common case

for await (const [message] of on(worker, 'message')) {
   ...
}
@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Jun 9, 2019

@Raynos lol, I had no idea we were in the same room for two days :) definitely say hi next time!

@mcollina mcollina force-pushed the mcollina:on-async-iterator branch from efcf82d to 0e518be Dec 22, 2019
@nodejs-github-bot

This comment has been minimized.

doc/api/events.md Outdated Show resolved Hide resolved
doc/api/events.md Outdated Show resolved Hide resolved
mcollina and others added 2 commits Dec 22, 2019
Co-Authored-By: Benjamin Gruenbaum <inglor@gmail.com>
Co-Authored-By: Benjamin Gruenbaum <inglor@gmail.com>
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 23, 2019

CI passed. I think this is ready to land?

mcollina added a commit that referenced this pull request Dec 23, 2019
Fixes: #27847
PR-URL: #27994
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@mcollina

This comment has been minimized.

Copy link
Member Author

mcollina commented Dec 23, 2019

Landed in 38a593b

@mcollina mcollina closed this Dec 23, 2019
@mcollina mcollina deleted the mcollina:on-async-iterator branch Dec 23, 2019
BridgeAR added a commit that referenced this pull request Jan 3, 2020
Fixes: #27847
PR-URL: #27994
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
targos added a commit that referenced this pull request Jan 14, 2020
Fixes: #27847
PR-URL: #27994
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@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.