max-mapper/concat-stream

Use standard, error-first callbacks

zenflow opened this issue · 4 comments

Drawn from #15. Let's discuss in a separate issue.

jonathanong on Dec 1, 2013

callback(err, data) - it's a callback, not an event listener, so imo it should have err as the first argument. however, err should always be null since concat-stream should never have any errors (i get #6 (comment)) unless we decide to throw errors when there are crazy typing issues. we can do crazy stuff like check listener.length but i'm not a fan of that either.

substack on Dec 23, 2013

cb(err, data) is annoying if there isn't ever an error. Why not just omit that parameter like it presently is?

jeromew on Jan 13, 2014

[...] don't understand "concat-stream should never have any errors" because shouln't concat-stream handle the errors of the underlying streams to report via cb(err, data) ?

zenflow on June 29, 2015 (just now)

If you pipe a readable stream to this or any other writable stream (with Readable.protototype.pipe), errors will not be piped downstream along with the data.
When using this library, you must handle upstream errors yourself
Besides any upstream errors, there are no other errors to expect.

I agree

@kumavis what in particular do you agree with?

good question. I was anticipating that upstream errors would flow through, and therefore there should be an error handler.

Can this stream close/end/finish? if it can, and then more data is pushed into it, shouldn't it error? The notion that this never errors seems misguided.

but im blowing smoke b/c i dont particularly remember ever actually used this module

I think pipeline errors should be dispatched, too.