grncdr / merge-stream

Merge multiple streams into one interleaved stream
MIT License
214 stars 16 forks source link

Errors are not propagated #24

Closed FredKSchott closed 7 years ago

FredKSchott commented 7 years ago

Currently, errors that are created inside of the mergeStream are not propagated out of it. As an example:

// If an error occurs in streamOne or streamTwo, neither reject nor resolve is called. 
// The stream simply hangs.
mergeStream(streamOne, streamTwo)
  .on('error', reject)
  .on('finish', resolve);

This is unexpected behavior to me and I suspect all users, since they have no way to consume/handle errors. Because I am merging multiple streams into one, I would expect that merge stream to propagate all errors that occur in the streams that make it up. Otherwise they are lost with no way to handle them.

This was originally filed in issue #13, but I believe the OP was asking for a way to propagate errors down any chain of Node.js streams. So I'm reopening here with a more specific focus to merge-stream.

@grncdr wdyt? Is there any reason that this shouldn't be default behavior?

stevemao commented 7 years ago

How would you know whether streamOne or streamTwo emits the error? Why is it better than

streamOne.on('error'...)
streamTwo.on('error'...)

mergeStream(streamOne, streamTwo)
  ...
FredKSchott commented 7 years ago

You can always add specific listeners like in your example, if you want to react to each stream differently.

The problem is that if you don't add specific error handling to each individual stream, the merged stream just times out. This is unexpected, and makes it hard to work with especially if you are a library exposing only the merged stream to the consumer. (This is something we'd like to do in my project, https://github.com/Polymer/polymer-build)

stevemao commented 7 years ago

Great explanation @FredKSchott !