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

Sink should respect a readable event on the destination stream #142

Closed
wants to merge 1 commit into from
Closed

Sink should respect a readable event on the destination stream #142

wants to merge 1 commit into from

Conversation

benurb
Copy link
Contributor

@benurb benurb commented Jan 12, 2016

Hello 😄

by using grunt-contrib-imagemin I encountered a problem that I tracked down to vinyl-fs 2.3.0. If one attaches a listener to the readable event of the vinly-fs.dest stream, the new sink (introduced in df97b14 ) prevents it from firing. I attached a test to this PR that hopefully explains the problem better.

The problem is that the pipe() call in sink.js turns the stream into flowing mode. In this mode the stream does no longer emit a readable event (see Node docs).

This of course is a special case. Nevertheless the destination stream should not be turned into flowing mode if it's obvious that this was not intended. Imho that's the case if a developer adds a readable event listener, so I added a check for it.

Feedback is much appreciated! 😄

Ben

@phated
Copy link
Member

phated commented Jan 12, 2016

This solution was suggested by @chrisdickinson. I'd like to get his feedback on your change.

@benurb
Copy link
Contributor Author

benurb commented Jan 12, 2016

Sure 👍
The main problem currently is that 2.3.0 introduces a breaking change in a minor version update. I guess not many people will notice this, but it's probably better if we can get this sorted out quickly.

@phated
Copy link
Member

phated commented Jan 12, 2016

Yep, but a breaking change is intentional, this was a regression. Sorry about that.

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

Successfully merging this pull request may close these issues.

None yet

2 participants