-
Notifications
You must be signed in to change notification settings - Fork 30k
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: support EventTarget in once
#29498
events: support EventTarget in once
#29498
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good mostly! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last iteration lgtm :]
Thanks for following up on #27977 hope to see you around again 🙏
@benjamingr @addaleax Thanks! |
@JeniaBR You might find |
d3938a4
to
836d846
Compare
Travis pass 👍 |
I think this needs a rebase, as it shows 361 commits. |
test/parallel/test-events-once.js
Outdated
@@ -84,10 +84,72 @@ async function onceError() { | |||
strictEqual(ee.listenerCount('myevent'), 0); | |||
} | |||
|
|||
async function onceWithEventTarget() { | |||
const emitter = new class EventTargetLike extends EventEmitter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the right approach. I would prefer this not to extend EventEmitter.
Also, you can make it a top level helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mcollina I thought about implementing partial EventTarget and use it for testing.
Something like that EventTarget Example (Also, need to implement in that example above once
functionality).
This approach seems to be a little bit exaggerated.
WDYT I should do here?
(If we decide to keep it like that, I will extract it to a top level helper)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 to have the EventTarget
example extend EventEmitter
. We do not need a full EventTarget
implementation, but we could just mock/place assertions on the methods that are used by once
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
lib/events.js
Outdated
errorListener = (err) => { | ||
emitter.removeListener(name, eventListener); | ||
reject(err); | ||
if (typeof emitter.addEventListener !== 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this to be on the positive side instead. "if is EventTarget, then ...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@mcollina How can I do it properly? I synchronized my master with upstream/master, then I tried to rebase my master with this branch and I get a lot of merge conflicts. |
event or that is rejected when the `EventEmitter` emits `'error'`. | ||
The `Promise` will resolve with an array of all the arguments emitted to the | ||
given event. | ||
|
||
Note: This method is intentionally generic and works with the web platform | ||
[EventTarget](WHATWG-EventTarget) interface, which have no special | ||
`'error'` event semantics and do not listen to the `'error'` event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something, an example showing the use of the EventTarget API would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { once } = require('events');
const EventTarget = require('../event-target-implementation')
async function run() {
const et = new EventTarget();
process.nextTick(() => {
et.dispatchEvent('myevent', 42);
});
const [value] = await once(et, 'myevent');
console.log(value);
}
run();
@jasnell something like that?
Not sure about this line const EventTarget = require('../event-target-implementation')
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can defer doing that to a future pull request. Mostly this is for web compatibility and for making EE more universal JavaScript compatible but our docs on the other hand mostly ignore that as far as I know.
You did a merge which we typically don't do in Node.js - the process is rather than having a merge commit we do a rebase which places the commits on top of all the commits to this point. The way to go about this would be cherry picking the relevant commits ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with an example in the docs added
@addaleax @jasnell @mcollina @benjamingr What's the semverity of this? Patch? Minor? |
@Trott minor on the safe side IMO, this doesn't really introduce an API and could very theoretically break things but in practice shouldn't. This could theoretically be seen as introducing an API. I am also fine with patch. |
PR-URL: #29498 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Landed in 34a61d5 |
PR-URL: #29498 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Notable changes: * crypto: * Add `oaepLabel` option #29489 * deps: * Update V8 to 7.7.299.11 #28918 * More efficient memory handling * Stack trace serialization got faster * The `Intl.NumberFormat` API gained new functionality * For more information: https://v8.dev/blog/v8-release-77 * events: * Add support for `EventTarget` in `once` #29498 * fs: * Expose memory file mapping flag `UV_FS_O_FILEMAP` #29260 * inspector: * New API - `Session.connectToMainThread` #28870 * process: * Initial SourceMap support via `env.NODE_V8_COVERAGE` #28960 * stream: * Make `_write()` optional when `_writev()` is implemented #29639 * tls: * Add option to override signature algorithms #29598 * util: * Add `encodeInto` to `TextEncoder` #29524 * worker: * The `worker_thread` module is now stable #29512
PR-URL: #29498 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Notable changes: * crypto: * Add `oaepLabel` option #29489 * deps: * Update V8 to 7.7.299.11 #28918 * More efficient memory handling * Stack trace serialization got faster * The `Intl.NumberFormat` API gained new functionality * For more information: https://v8.dev/blog/v8-release-77 * events: * Add support for `EventTarget` in `once` #29498 * fs: * Expose memory file mapping flag `UV_FS_O_FILEMAP` #29260 * inspector: * New API - `Session.connectToMainThread` #28870 * process: * Initial SourceMap support via `env.NODE_V8_COVERAGE` #28960 * stream: * Make `_write()` optional when `_writev()` is implemented #29639 * tls: * Add option to override signature algorithms #29598 * util: * Add `encodeInto` to `TextEncoder` #29524 * worker: * The `worker_thread` module is now stable #29512 PR-URL: #29695
Notable changes: * crypto: * Add `oaepLabel` option #29489 * deps: * Update V8 to 7.7.299.11 #28918 * More efficient memory handling * Stack trace serialization got faster * The `Intl.NumberFormat` API gained new functionality * For more information: https://v8.dev/blog/v8-release-77 * events: * Add support for `EventTarget` in `once` #29498 * fs: * Expose memory file mapping flag `UV_FS_O_FILEMAP` #29260 * inspector: * New API - `Session.connectToMainThread` #28870 * process: * Initial SourceMap support via `env.NODE_V8_COVERAGE` #28960 * stream: * Make `_write()` optional when `_writev()` is implemented #29639 * tls: * Add option to override signature algorithms #29598 * util: * Add `encodeInto` to `TextEncoder` #29524 * worker: * The `worker_thread` module is now stable #29512 PR-URL: #29695
Really excited to see more browser interop in Node One thing that worries me is that this prevents I would have expected the check to be reversed: If the object is a Node I also had a use case for |
// EventEmitters, we do not listen to `error` events here. | ||
emitter.addEventListener( | ||
name, | ||
(...args) => { resolve(args); }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EventTarget is guaranteed to only have one argument, so you could avoid passing an array here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domenic what do you think is the right call API wise in terms of compatibility with EventEmitter and the coming (?) events proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't pay much attention to early-stage TC39 proposals. But I think just compatibility with how events work in event listeners in the DOM would suggest using a single argument instead of an array containing that argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this now resolves with an array, and just got released in Node v12.11.0. Are we now stuck with this promise signature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this now resolves with an array, and just got released in Node v12.11.0. Are we now stuck with this promise signature?
I think changing that would be a breaking change, yes.
That being said, I think there is some value in keeping this consistent between EventTarget and EventEmitter, so I’m not really sure that we should change this even if we could.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about consistency with web EventTarget? If that is a goal, arrays should not get involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about consistency with web EventTarget? If that is a goal, arrays should not get involved.
@domenic Yes, there’s a tradeoff, but at least to me a polymorphic API seems worse here than one that performs some unnecessary boxing.
That being said: I personally don’t like the decision to use arrays in the first place, but I think we’re stuck with that now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I'm looking at this and I'm pretty sure Domenic was right and our implementation is wrong. Even for consistency.
Our tests incorrectly use an EventTargetMock that accepts multiple arguments (which isn't possible) and dispatches strings (and not objects).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #33659
@felixfbecker that sounds very reasonable to me and was simply missed. @JeniaBR would you be willing to put in the work to change the order so that the API checks if the passed in emitter is a Node EventEmitter first? |
@felixfbecker good point! |
My attempt to add support for EventTarget in the once static method.
This PR follow up #27977
Ping @benjamingr please take a look.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes