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

Read data asynchronously #14

Closed
wants to merge 1 commit into from

Conversation

lukechilds
Copy link
Contributor

Doesn't block while waiting for stdin to finish.

Discussed in #13

@SamVerschueren
Copy link

Could you rebase from master?

@lukechilds
Copy link
Contributor Author

lukechilds commented Jun 6, 2016

Am doing, gimme a second, jeez.

@lukechilds
Copy link
Contributor Author

Sorted 😉

@SamVerschueren
Copy link

Sorry, didn't want to put pressure on you :).

@lukechilds
Copy link
Contributor Author

Haha, no worries man I was just kidding.

@kevva
Copy link
Contributor

kevva commented Jun 7, 2016

It's already async (AFAIK), and this code is intentional. See eedcea4.

@lukechilds
Copy link
Contributor Author

Are you sure? Does while (chunk = stdin.read()) not block?

@lukechilds
Copy link
Contributor Author

When get-stdin subscribe to stdin, it blocks the process until something is written to stdin.

#13 (comment)

@lukechilds
Copy link
Contributor Author

lukechilds commented Jul 8, 2016

@SamVerschueren @kevva I just had another look at this and it is indeed currently async.

The difference is with the readable event (introduced in Streams2) you can choose whether to read the data now or later. With data you have to handle the data in the callback or you lose it forever. The current code uses the more advanced readable event but always handles the data straight away anyway.

In effect it's kind of using the new Streams2 api readable event to emulate the behaviour of the old data event. Although they should both have the same behaviour I think it makes more sense to merge this PR and revert back to data. It's much more readable, and more clearly shows the intentions of this code, as demonstrated by the confusion in this PR and the related issue. 😆

Up to you guys :)

@sindresorhus
Copy link
Owner

sindresorhus commented Dec 2, 2016

I guess, but I don't want to take the chance. Not worth potential breakage. Too many modules depend on this one. The current version should also be slightly faster as event emitting is slower than reading from a function, and the version in master uses a lot less events.

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.

4 participants