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.

Sign up for GitHub

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

Jump to bottom

events: support EventTarget in once #27977

Closed
wants to merge 8 commits into from
Closed

events: support EventTarget in once #27977

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
de50efd
events: support EventTarget in `once`
benjamingr May 30, 2019
8cd36cd
Update test/parallel/test-events-once.js
benjamingr May 31, 2019
cb5fecd
cr + docs
benjamingr May 31, 2019
99117f8
error event semantics docs
benjamingr May 31, 2019
774f06e
Update doc/api/events.md
benjamingr May 31, 2019
fc09d9a
Update doc/api/events.md
benjamingr May 31, 2019
6fc65f0
fixes
benjamingr May 31, 2019
c409d35
Merge branch 'event-emitter-once-event-target' of github.com:benjamin…
benjamingr May 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion doc/api/events.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,9 +663,13 @@ added: v11.13.0

Creates a `Promise` that is resolved when the `EventEmitter` emits the given
event or that is rejected when the `EventEmitter` emits `'error'`.
The `Promise` will resolve with an array of all the arguments emitted to the
The `Promise` will fulfill with an array of all the arguments emitted to the
given event.

Note: This method is intentionally generic and works with web platform
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
[EventTarget](WHATWG-EventTarget) instances. `EventTarget`s have no special
benjamingr marked this conversation as resolved.
Show resolved Hide resolved
`'error'` event semantics and do not listen to the `'error'` event.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth mentioning the different argument semantics? It resolves with the Event instance rather than an array of arguments.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski I was not sure about what to do here - what behavior would you think is less surprising?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More saying we should make clear the distinction in arguments that the callback receives when you use this with EventTarget.


```js
const { once, EventEmitter } = require('events');

Expand Down Expand Up @@ -703,4 +707,5 @@ run();
[`fs.ReadStream`]: fs.html#fs_class_fs_readstream
[`net.Server`]: net.html#net_class_net_server
[`process.on('warning')`]: process.html#process_event_warning
[WHATWG-EventTarget](https://dom.spec.whatwg.org/#interface-eventtarget)
[stream]: stream.html
13 changes: 12 additions & 1 deletion lib/events.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,17 @@ function once(emitter, name) {
emitter.once('error', errorListener);
}

emitter.once(name, eventListener);
if (typeof emitter.once === 'function' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be going totally crazy but I feel like this function is incorrect and the test only passes because it runs through the EventEmitter path? I think you need something more like this?

function once(emitter, name) {
  return new Promise((resolve, reject) => {
    if (typeof emitter.once === 'function' &&
        typeof emitter.removeListener === 'function') {
      const eventListener = (...args) => {
        if (errorListener !== undefined) {
          emitter.removeListener('error', errorListener);
        }
        resolve(args);
      };
      let errorListener;

      // Adding an error listener is not optional because
      // if an error is thrown on an event emitter we cannot
      // guarantee that the actual event we are waiting will
      // be fired. The result could be a silent way to create
      // memory or file descriptor leaks, which is something
      // we should avoid.
      if (name !== 'error') {
        errorListener = (err) => {
          emitter.removeListener(name, eventListener);
          reject(err);
        };

        emitter.once('error', errorListener);
      }

      emitter.once(name, eventListener);
      return;
    }

    if (typeof emitter.addEventListener === 'function') {
      // EventTarget does not have `error` event semantics like Node
      // EventEmitters, we do not listen to `error` events here.
      emitter.addEventListener(name, resolve, { once: true });
      return;
    }

    throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter);
  });
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the difference here - but this can be a brainfart on my behalf

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your original PR still binds the error listener (and unnecessarily creates the eventListener function). It also creates a nested Promise that it then resolves so the original never resolves. I could also be missing something... but when I ran the code, it didn't seem to do what you're expecting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski ah wow you're right!

typeof emitter.removeListener === 'function') {
return emitter.once(name, eventListener);
}
if (typeof emitter.addEventListener === 'function') {
return new Promise((resolve) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra Promise not needed here?

// Although EventTarget does not have `error` event semantics like Node
// EventEmitters, we do not listen to `error` events here.
emitter.addEventListener(name, resolve, { once: true });
});
}
throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter);
});
}
21 changes: 20 additions & 1 deletion test/parallel/test-events-once.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,29 @@ async function onceError() {
strictEqual(ee.listenerCount('myevent'), 0);
}

async function onceWithEventTarget() {
const emitter = new class EventTargetLike extends EventEmitter {
addEventListener = common.mustCall(function (name, listener, options) {
if (options.once) {
this.once(name, listener);
} else {
this.on(name, listener);
}
});
};
Copy link
Member

@apapirovski apapirovski May 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add emitter.removeListener = undefined; to hit the right code path in this test?

process.nextTick(() => {
emitter.emit('myevent', 42);
});
const [value] = await once(emitter, 'myevent');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check that there are no interesting error semantics?

strictEqual(value, 42);
strictEqual(emitter.listenerCount('myevent'), 0);
}

Promise.all([
onceAnEvent(),
onceAnEventWithTwoArgs(),
catchesErrors(),
stopListeningAfterCatchingError(),
onceError()
onceError(),
onceWithEventTarget()
]).then(common.mustCall());