events: support EventTarget in `once` by jeniabrook · Pull Request #29498 · nodejs/node · GitHub

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), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added

Looks good mostly! :)

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

Last iteration lgtm :]

Thanks for following up on #27977 hope to see you around again 🙏

@benjamingr @addaleax Thanks!
How can I fix the commit messages in order to pass Travis CI? :)

@JeniaBR You might find core-validate-commit to be of help with that.

Travis pass 👍
What's next? @addaleax @benjamingr @mscdex

I think this needs a rebase, as it shows 361 commits.

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.

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

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.

done.

I would prefer this to be on the positive side instead. "if is EventTarget, then ...".

done.

I think this needs a rebase, as it shows 361 commits.

@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.
I think I'm doing something wrong here.

Unless I'm missing something, an example showing the use of the EventTarget API would be helpful.

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

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.

I think I'm doing something wrong here.

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 (git cherry-pick COMMIT) on top of checked out master. Alternatively you can just re-apply the changes on top of master (I am not concerned about attribution for my part and you can just re-apply the changes on master).

LGTM with an example in the docs added

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

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

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

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

Really excited to see more browser interop in Node ☺️

One thing that worries me is that this prevents once to listen to error for any kind of interop emitter that implements/extends both EventEmitter and EventTarget (and does so in a minor version).

I would have expected the check to be reversed: If the object is a Node EventEmitter, listen to error, otherwise not, but in the shipped version EventTarget is checked for first. I actually kind of would have hoped for Node's EventEmitterr to one day implement EventTarget for interop.

I also had a use case for error events on the web come to mind: When you open a WebSocket, you'll want to listen and wait for the open event to know when to start sending messages. If the connection fails during that wait (e.g. the server could not be reached) the error event will fire instead. You'll want to make sure these listeners are also cleaned up properly. The only difference is that it fires an Event object, not an Error object, which in my eyes is enough of an argument to not listen to it in once.

EventTarget is guaranteed to only have one argument, so you could avoid passing an array here.

@domenic what do you think is the right call API wise in terms of compatibility with EventEmitter and the coming (?) events proposal?

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.

So this now resolves with an array, and just got released in Node v12.11.0. Are we now stuck with this promise signature?

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.

What about consistency with web EventTarget? If that is a goal, arrays should not get involved.

@domenic @addaleax I thought about return one argument, but for consistency with EventEmitter decide to keep the array.

I will open soon a new PR with some improvements.
We can go with EventTarget API and I will update the code, docs and the tests.

WDYT?

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.

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

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!
@benjamingr sure, I will do it.

Port of nodejs/node#29498

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

mscdex reviewed Sep 8, 2019

View reviewed changes

Comment thread doc/api/events.md Outdated

mscdex reviewed Sep 8, 2019

View reviewed changes

Comment thread lib/events.js Outdated

addaleax reviewed Sep 8, 2019

View reviewed changes

Comment thread doc/api/events.md Outdated

Comment thread lib/events.js Outdated

jeniabrook requested review from addaleax, benjamingr, devsnek and mscdex September 10, 2019 19:24

mscdex reviewed Sep 10, 2019

View reviewed changes

Comment thread lib/events.js Outdated

addaleax approved these changes Sep 10, 2019

View reviewed changes

benjamingr approved these changes Sep 11, 2019

View reviewed changes

benjamingr mentioned this pull request Sep 11, 2019

events: support EventTarget in once #27977

Closed

jeniabrook force-pushed the event-emitter-once-event-target branch 2 times, most recently from d3938a4 to 836d846 Compare September 11, 2019 07:58

mcollina reviewed Sep 11, 2019

View reviewed changes

jeniabrook requested a review from mcollina September 11, 2019 17:04

jasnell reviewed Sep 11, 2019

View reviewed changes

Comment thread doc/api/events.md Outdated

jasnell reviewed Sep 11, 2019

View reviewed changes

Comment thread lib/events.js Outdated

jasnell reviewed Sep 11, 2019

View reviewed changes

jasnell approved these changes Sep 11, 2019

View reviewed changes

jeniabrook added 3 commits September 12, 2019 10:05

events: add support for EventTarget in once

c48786e

events: add test onceWithEventTarget

0b69f8b

events: add handling for error

1a5f42d

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:50

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 18, 2019

Trott closed this Sep 23, 2019

BridgeAR mentioned this pull request Sep 24, 2019

v12.11.0 proposal #29695

Merged

domenic reviewed Sep 26, 2019

View reviewed changes

benjamingr mentioned this pull request May 30, 2020

events: on EventTarget support + once compliance #33659

Closed

4 tasks

goto-bus-stop added a commit to browserify/events that referenced this pull request Feb 27, 2021

add support for EventTarget in once

2a68899

Port of nodejs/node#29498

goto-bus-stop mentioned this pull request Feb 27, 2021

add support for EventTarget in once browserify/events#82

Merged

davidbabcock96 added a commit to davidbabcock96/modern-events that referenced this pull request Mar 3, 2026

add support for EventTarget in once

b6bf4a4

Port of nodejs/node#29498

josephjorden93 added a commit to POWEREVENTS/power-events that referenced this pull request Mar 10, 2026

add support for EventTarget in once

c4a60e8

Port of nodejs/node#29498

+                    // EventEmitters, we do not listen to `error` events here.
+                    emitter.addEventListener(
+                      name,
+                      (...args) => { resolve(args); },

events: support EventTarget in `once` by jeniabrook · Pull Request #29498 · nodejs/node · GitHub

Uh oh!

Uh oh!

events: support EventTarget in once - #29498

events: support EventTarget in once#29498jeniabrook wants to merge 12 commits intonodejs:masternodejs/node:masterfrom jeniabrook:event-emitter-once-event-targetjeniabrook/node:event-emitter-once-event-targetCopy head branch name to clipboard

Conversation

jeniabrook commented Sep 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 10, 2019

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

jeniabrook commented Sep 11, 2019

Uh oh!

mscdex commented Sep 11, 2019

Uh oh!

jeniabrook commented Sep 11, 2019

Uh oh!

mcollina commented Sep 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeniabrook commented Sep 11, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Sep 11, 2019

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 18, 2019

Uh oh!

benjamingr commented Sep 18, 2019

Uh oh!

nodejs-github-bot commented Sep 18, 2019

Uh oh!

Trott commented Sep 23, 2019

Uh oh!

felixfbecker commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronkorving Sep 30, 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!

Choose a reason for hiding this comment

events: support EventTarget in once#29498
jeniabrook wants to merge 12 commits into
nodejs:masterfrom
jeniabrook:event-emitter-once-event-target

jeniabrook commented Sep 8, 2019 •
edited
Loading

felixfbecker commented Sep 26, 2019 •
edited
Loading

ronkorving Sep 30, 2019 •
edited
Loading