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

pipe works but through doesn't? #449

Closed
ronag opened this issue Feb 8, 2016 · 7 comments
Closed

pipe works but through doesn't? #449

ronag opened this issue Feb 8, 2016 · 7 comments
Labels

Comments

@ronag
Copy link

ronag commented Feb 8, 2016

Using the following library https://github.com/shovon/bpg-stream

This works but doesn't propagate errors:

_(...)
  // .on('error', ...) // We don't want this...
  .pipe(new BPGStream())
  .on('error', ...)
  .pipe(res)

While this never ends/finishes:

_(...)
  .through(new BPGStream())
  .on('error', ...)
  .pipe(res)

What further requirements does through have over pipe?

@jeromew
Copy link
Collaborator

jeromew commented Feb 8, 2016

Does the second version start at all ?

The implementations of though and pipe are totally different

pipe does not forward errors to behave like a standard Readable.

In the through case it would be interesting to understand why it never ends. I suppose this is an interop issue with the way BPGStream ends the stream. Maybe you can activate the debug mode of BPGStream to see if https://github.com/shovon/bpg-stream/blob/master/index.js#L208 is reached.

I try to avoid mixin Node streams with highland streams because there seem to be hard to debug edge cases (interop with node streams is hard).

I have had some success using barrage to go from the highland world to the node stream world or to the Promise world

In your case that would be

npm install barrage

var h = _(...)
barrage(h)
.syphon(new BPGStream())
.on('error', ...)
.pipe(res)

@ronag
Copy link
Author

ronag commented Feb 8, 2016

Does the second version start at all ?

I don't quite know. I know it doesn't end.

pipe does not forward errors to behave like a standard Readable.

Yes, I've understood this. Which is why I'd like to use through

I have had some success using barrage to go from the highland world to the node stream world or to the Promise world

I'll try that. Thanks!

@ronag
Copy link
Author

ronag commented Feb 8, 2016

Tried this as well without success.

  const out = _()
  const stdout = _(...)
    .on('error', err => out.write(err))
    .pipe(new BPGStream())
    .on('error', err => out.write(err))
    .pipe(out)

@vqvu
Copy link
Collaborator

vqvu commented Feb 8, 2016

Highland's pipe implementation doesn't emit('pipe') (this is probably a bug). I wonder if that's the problem... Though if it was, then I don't understand why your first example using only pipe works.

@ronag
Copy link
Author

ronag commented Feb 8, 2016

@vque: That's because my first example is wrong. My apologies, it should be:

_(...
  // .on('error', ...) // We don't want this...
  .pipe(new BPGStream()))
  .on('error', ...)
  .pipe(res)

@vqvu vqvu added the bug label Feb 8, 2016
@vqvu
Copy link
Collaborator

vqvu commented Feb 8, 2016

That's it then. Looks like bpg-stream listens for pipe so that it can do on('end') on the source stream, which is kind of silly since the source ending doesn't mean the internal fs stream that bpg-stream uses has finished writing.

Can you try running this?

_(...).through(s => {
        const bpgStream = new BPGStream()
        const res = s.through(bpgStream);
        bpgStream.emit('pipe', s);
        return res;
    })
    .pipe(...)

I tried to do it myself, but I don't have bpgenc installed.

@ronag
Copy link
Author

ronag commented Feb 8, 2016

@vqvu: Yes, that solved it.

vqvu added a commit to vqvu/highland that referenced this issue Feb 9, 2016
- `pipe` now emits the `pipe` event on the destination when piping.
- `pipe` has an optional pipe options argument that allows users to
  choose to not end the destination when the source ends.
vqvu added a commit to vqvu/highland that referenced this issue Feb 9, 2016
- `pipe` now emits the `pipe` event on the destination when piping.
- `pipe` has an optional pipe options argument that allows users to
  choose to not end the destination when the source ends.
vqvu added a commit to vqvu/highland that referenced this issue Feb 9, 2016
- `pipe` now emits the `pipe` event on the destination when piping.
- `pipe` has an optional pipe options argument that allows users to
  choose to not end the destination when the source ends.
@vqvu vqvu closed this as completed in 25f8051 Feb 13, 2016
vqvu added a commit that referenced this issue Feb 13, 2016
Make pipe behave more like Readable#pipe. Fixes #449.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants