Skip to content

Add onceDrain #28

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

Closed
wants to merge 1 commit into from
Closed

Add onceDrain #28

wants to merge 1 commit into from

Conversation

rnons
Copy link

@rnons rnons commented Feb 9, 2020

Related to #15

It seems more common to use .once('drain', cb) than .on('drain', cb), at least on https://nodejs.org/dist/latest-v12.x/docs/api/stream.html, there are two .once('drain', cb) examples. Like

if (!stream.write(data)) {
    stream.once('drain', cb);
  } 

If you prefer .on('drain', cb) or both, I will update.

@hdgarrood
Copy link
Member

I think the desire to bind to once indicates that we should rethink how we are binding to event emitters here actually. For each event, we often only provide a binding to on(evt, callback) but there's quite a lot of the node.js event emitter API which we aren't covering: https://nodejs.org/dist/latest-v12.x/docs/api/events.html. I would personally rather hold off on adding this and instead think about putting together a proper binding to EventEmitter, like https://github.com/joneshf/purescript-node-events/

@hdgarrood
Copy link
Member

No, it probably shouldn't: I am saying that reproducing the whole EventEmitter API for every node API which uses events is undesirable.

@hdgarrood
Copy link
Member

Potentially, yes. I think we should at least investigate that approach first.

@JordanMartinez
Copy link
Contributor

Superceded by #49

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