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

Promisified equivalent of EventEmitter.once() #20893

Closed
kirly-af opened this issue May 22, 2018 · 9 comments
Closed

Promisified equivalent of EventEmitter.once() #20893

kirly-af opened this issue May 22, 2018 · 9 comments

Comments

@kirly-af
Copy link

Hi,

Right now we can subscribe to one time events using .once(). Because most (all?) Node event emitters emit an "error" event when an error occurs, it seems reasonable to wrap such events in a Promise. For example:

const EventEmitter = require('events')
const myEmitter = new EventEmitter()

myEmitter.oncePromise('ready')
  .then(() => console.log('end event fired'))

myEmitter.emit('load')
myEmitter.emit('ready')

The name oncePromise is probably not the best but you get the idea.
Given the following implementation:

const EventEmitter = require('events')
EventEmitter.prototype.oncePromise = function(completeEvent, errorEvent = 'error') {
  return new Promise((resolve, reject) => {
    this.once(completeEvent, resolve).once(errorEvent, reject)
  })
}

That way we could leverage async/await flow and error handling:

const myEmitter = new EventEmitter()

function foo() {
  setTimeout(() => myEmitter.emit('error', new Error('foo')), 500)
}

function bar() {
  console.log('bar')
}

(async () => {
  try {
    foo()
    await myEmitter.oncePromise('ready')
    bar() // never gets called
  } catch (err) {
    console.log(err) // "foo"
  }
})()

Existing solutions:

@benjamingr
Copy link
Member

If we decide it's a good idea I'm willing to go ahead and make a PR, I have a branch with this sitting anyway.

@benjamingr
Copy link
Member

It's basically:

EventEmitter.prototype.once = function once(type, listener) {
  if (typeof listener !== 'function') {
    return new Promise((resolve) => this.on(type, _onceWrap(this, type, resolve));
    // the old behavior was to fire an error on an invalid arg type - so we 
    // can count on `once` with a single argument to be free for the taking.
    // old behavior:
    // const errors = lazyErrors();
    // throw new errors.ERR_INVALID_ARG_TYPE('listener', 'Function', listener);
  }
 
  this.on(type, _onceWrap(this, type, listener));
  return this;
};

@kirly-af
Copy link
Author

@benjamingr That's great, though your implementation does not take care of rejecting the Promise when the error event is fired.

Also I wonder if it's not error prone that '.once' have such different return types depending on the second argument. Might be interesting to consider using a different method name.

@benjamingr
Copy link
Member

benjamingr commented May 23, 2018

That's great, though your implementation does not take care of rejecting the Promise when the error event is fired.

I don't think we should do that since event emitters are very general and also because when there is no error event handler Node.js terminates by default - so adding an event handler for error implicitly would be very surprising behavior in Node.

Also I wonder if it's not error prone that '.once' have such different return types depending on the second argument. Might be interesting to consider using a different method name.

I did but it turns out this is backwards compatible because the current behavior for the second argument is to throw a ERR_INVALID_ARG_TYPE .

benjamingr pushed a commit to benjamingr/io.js that referenced this issue May 23, 2018
This commit introduces a `once` overload which returns a promise if a
listener isn't passed to `once`.

Fixes: nodejs#20893

PR-URL:
Reviewed-By:
@benjamingr
Copy link
Member

I've made a PR @kirly-af to get the discussion going.

@DaAitch
Copy link
Contributor

DaAitch commented May 24, 2018

A oncePromise would not add additional features to a very basic EventEmitter only syntactic sugar. I feel like there is no huge benefit. Also the correlation between any event and 'error' is just a convention. I think an 'event promisify' library like util.promisify does it with callbacks is where to find such tools, but EventEmitter itself should not have any impl based on conventions (my opinion).

@benjamingr
Copy link
Member

Hey @DaAitch thanks for weighing in, would you mind taking a look at my post here #20909 (comment) and the discussion there with interesting points from several collaborators?

I'm wondering if that would make my motivation for considering this clear.

Also - how do you think we can have a better async/await story?

@simonkcleung
Copy link

simonkcleung commented May 25, 2018

This should work:

const EventEmitter = require('events');
const myEmitter = new EventEmitter();

EventEmitter.prototype.await = function(resolveEvent, rejectEvent) {

  var _resolve,
      _reject;
  var promise = new Promise( (resolve, reject) => {
    _resolve = resolve;
    this.on(resolveEvent, resolve);
    if (rejectEvent) {
      _reject = reject;
      this.on(rejectEvent, reject);
	}
  });
	
  return rejectEvent ? promise.finally( () => {
	  this.off(resolveEvent, _resolve);
	  this.off(rejectEvent, _reject);
	}):	promise.finally(() => {
       this.off(resolveEvent, _resolve);
    });
}

myEmitter.await('ready')
  .then(() => console.log('ready1'));

myEmitter.emit('ready'); // 'ready1' 

myEmitter.await('ready', 'abort')
  .then(() => {console.log('ready2')}, () => {console.log("aborted2")});

myEmitter.emit('ready'); // 'ready2'
myEmitter.emit('abort'); // nothing

myEmitter.await('ready', 'abort')
  .then(() => {console.log('ready3')}, () => {console.log("aborted3")});

myEmitter.emit('abort'); // aborted3

@cjihrig
Copy link
Contributor

cjihrig commented Mar 5, 2019

I believe this can be closed by #26078.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants