browserify / events

Node's event emitter for all engines.
MIT License
1.38k stars 173 forks source link

Add events.once #63

Closed goto-bus-stop closed 4 years ago

goto-bus-stop commented 5 years ago

Node.js 11.13 added an EventEmitter.once method: https://nodejs.org/api/events.html#events_events_once_emitter_name

It returns a Promise that resolves when the requested event is fired. If an 'error' event is fired first, the Promise rejects with the error.

The path to implementing this looks a bit like:

We can then release this as a minor version.

vvscode commented 5 years ago

@goto-bus-stop as far as I see - there is already some .once implementation. And the existing one is incompatible with a new description

https://github.com/Gozala/events/blob/master/tests/once.js

vweevers commented 5 years ago

@vvscode That's require('events').EventEmitter.prototype.once, rather than require('events').once

goto-bus-stop commented 5 years ago

Yes, there is already a EventEmitter.prototype.once method that adds an event listener:

var emitter = new EventEmitter()
emitter.once('xyz', function (arg1, arg2) {
})

This is for the new EventEmitter.once method that returns a Promise instead:

var emitter = new EventEmitter()
var [arg1, arg2] = await EventEmitter.once(emitter, 'xyz')
vvscode commented 5 years ago

Ok. A frew more questions - don't you use any build system for https://github.com/Gozala/events/blob/master/events.js ?

Is it required copy-paste tests or I can write to them by myself?

goto-bus-stop commented 5 years ago

We don't have any build system for events.js. The tests are bundled using browserify but without babel or anything, so they should work in old browsers as much as possible (eg always use .then() instead of await).

The goal is to have identical behaviour to Node.js, so I think our tests should at least be based on Node's. they can be rewritten a bit (and have to be ported to older JS) but they should test the same functionality.

vweevers commented 5 years ago

You can pretty much copy https://github.com/nodejs/node/pull/26078/files, there have been no changes to the once function since that PR

Doogiemuc commented 5 years ago

Most recent version of NodeJS v12.10.0 changed the order of arguments for theoncemethod: https://nodejs.org/api/events.html

goto-bus-stop commented 5 years ago

@Doogiemuc hmm, I don't see it? that would break the entire ecosystem so I hope they wouldn't do that :) There are two .once() functions: the long-time emitter.once(eventName, callback) method and the newer EventEmitter.once(emitter, eventName) associated function.

vweevers commented 5 years ago

It really should have gotten a distinct name 😄