consume not passing errors

[mpj]

Given the code below, I except the console.error to output. Instead, the error is thrown (see error below)

var _ = require('highland');

var brokenThroughStream = _().consume(function(err, x, push, next) {
  push(new Error('I BROKE'), null)
})

var a = _();

var b = a.through(brokenThroughStream)

b
  .errors(function(err){
    console.error('I will never output :(')
  })
  .resume()

a.write("something")

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: I BROKE
    at /Users/mpj/code/request-through/error-handling-through.js:4:8
    at Stream.s._send (/Users/mpj/code/request-through/node_modules/highland/lib/index.js:1021:9)
    at Stream.write (/Users/mpj/code/request-through/node_modules/highland/lib/index.js:1094:18)
    at /Users/mpj/code/request-through/node_modules/highland/lib/index.js:790:23
    at Stream.s._send (/Users/mpj/code/request-through/node_modules/highland/lib/index.js:1021:9)
    at Stream.write (/Users/mpj/code/request-through/node_modules/highland/lib/index.js:1094:18)
    at Stream._send (/Users/mpj/code/request-through/node_modules/highland/lib/index.js:571:19)
    at Stream.write (/Users/mpj/code/request-through/node_modules/highland/lib/index.js:1094:18)
    at Object.<anonymous> (/Users/mpj/code/request-through/error-handling-through.js:17:3)
    at Module._compile (module.js:456:26)

[LewisJEllis]

This has nothing to do with .consume. You're passing the error to .pipe (inside .through), but .pipe causes a thunk. Anything that causes a thunk will emit any errors passed to it. This is happening here and is expected behavior.

[jeromew]

Indeed, there are 2 branches in through, depending on whether the target is a function or already a stream. @mpj your code would work as expected with

var brokenThroughStream = _.consume(function(err, x, push, next) {
  push(new Error('I BROKE'), null)
})

@LewisJEllis the fact that pipe re-emits the error is what is coded but when only working with highland streams it feels unnatural to me. This part probably comes from interactions with native node streams and I am not what we should target for the implementation.

[LewisJEllis]

Pipe could certainly take a shortcut when dealing only with highland streams. This is related to the idea I had here before realizing that the function branch is enough for that usage.

It would make piping between highland streams faster/simpler, but what are the drawbacks?

[jeromew]

I think the current situation comes from the fact that pipe was implemented with node streams in mind as a target.

The documentation for pipe explicitely says "Pipes a Highland Stream to a Node Writable Stream (Highland Streams are also Node Writable Streams)" - http://highlandjs.org/#pipe

through "will always return a Highland Stream (instead of the piped to target directly as in Node.js)." - http://highlandjs.org/#through

this can be seen in the code where we have

var output = _();
target.pause();
this.pipe(target).pipe(output);
return output;

Since through always upgrades streams to the highland world, It would make sense to me that through would catch all the errors encountered in target and re-inject them in the output as highland errors

Nevertheless, I believe such a change in behavior cannot be envisioned without a +1 from @caolan

[LewisJEllis]

Yep, all makes sense, and none of that precludes this potential shortcut; it would still satisfy the agreement to work on any Node Writable Stream.

Catching errors off the first pipe is an interesting idea, but certainly a significant behavior change. The shortcut would already make through pass errors along when target is a highland stream, and I think I like the idea of also doing it for non-highland targets since we know output is a highland stream. Interested to hear what Caolan thinks.

[caolan]

Provided we always emit an 'error' event (therefore throwing if no handler present) in the case of piping to a node stream I'm happy to add a case for piping to highland streams which passes the error downstream as you might expect. So @jeromew that's a +1 ;) - we'll have to decide if this constitutes a major version bump though.

[vqvu]

I've been thinking about this because of the PR that @svozza recently submitted.

Isn't the problem here that "through doesn't propagate errors like Highland streams" and not "pipe doesn't propagate errors like Highland streams"? So then isn't the solution to reimplement through so that it works (even if we have to stop implementing it with pipe.

It seems to me that pipe is imitating node stream pipes, which normally throws if there are no handlers registered. through is the highland version of that, which will propagate errors. We shouldn't break one to fix the other.

Isn't it weird behavior to have some node writable streams throw and some don't (highland streams) when passed to pipe?

[vqvu]

I think not changing pipe also has the added advantage of not breaking back compat (if we consider through throwing a bug), so we don't need to wait until 3.0.

[LewisJEllis]

+1; one sticking point was the inconsistency between hl#pipe and node#pipe, and this change would unify that behavior /and/ solve the original problem with through.