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

Allow for once to return a promise #171

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link

To start some discussion regarding nodejs/node#20909 (comment)

@lpinca
Copy link
Member

lpinca commented May 23, 2018

I'm not a fan of this, apart from the simplified usage offered by await, what is the real benefit?
cc: @3rd-Eden

@3rd-Eden
Copy link
Member

I've been following the discussion around this, and while I applaud the effort modernize the EventEmitter pattern to become fully async aware I don't think that this is way to go about it. It is weird, that a single method of the EventEmitter is upgraded to support async, especially if this be solved in userland in 5~ LOC as well:

function wonce(emitter, event) {
  return new Promise(function (resolve) {
    emitter.once(event, resolve)
  })
}

const data = await wonce(server, 'connected');

So for me this, the particular change would be -1, unless Node.js decides to implement it, then we will add support for it as well. Id be much more interested to see if we can make a fully async aware EventEmitter, instead of just spicing up a single method. Maybe work can be done around async iterators/generators for the on method. e.g:

for await (const data = stream.on('data')) {
  ...
}

Or otherways to make EventEmitter fully async, instead of just a single method.

@benjamingr
Copy link
Author

benjamingr commented May 24, 2018

especially if this be solved in userland in 5~ LOC as well:

The reason I am proposing this is because I've seen this seemingly simple implementation done wrong several times - when people have to write new Promise they end up having to put logic there.

What do you think about my example here: nodejs/node#20909 (comment) and the discussion here: nodejs/node#20909 (comment) ?

Maybe work can be done around async iterators/generators for the on method. e.g:

Node.js 10 already ships with Symbol.asyncIterator for streams - you can for await streams in Node.js today. If this excites you - we need help!

Quoting our docs:

const fs = require('fs');

async function print(readable) {
  readable.setEncoding('utf8');
  let data = '';
  for await (const k of readable) {
    data += k;
  }
  console.log(data);
}

print(fs.createReadStream('file')).catch(console.log);

This also has full backpressure support.

@lpinca
Copy link
Member

lpinca commented Apr 12, 2019

I'm closing this, but we should add https://nodejs.org/api/events.html#events_events_once_emitter_name.

@lpinca lpinca closed this Apr 12, 2019
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

Successfully merging this pull request may close these issues.

3 participants