max-mapper/concat-stream

v2 ideas

jonathanong opened this issue · 4 comments

you asked to help maintain this library, so here are some thoughts:

  • streams2 implementation, shouldn't be hard with readable-stream.
  • i don't like the type inferencing in getBody. prefer if it were like .pipe(cat.string()), .pipe(cat.buffer()), etc. by default, .pipe(cat()) should just return an array of all the things. developers should know the type of source stream and how they wish to consume the data.
  • delegate specific use-cases to other libraries. for example, if you want to concat to a string, just use raw-body (if we get .pipe support). overkill, but i don't feel like reimplementing the stream decoder stuff.
  • add cat(stream, callback), basically just stream.pipe(this)
  • 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.
  • don't re-emit data (#11). it doesn't handle back pressure so it's a terrible idea. people should just pipe to both cat-stream and the final stream.
  • yield stream.pipe(cat()) support. not sure how to do that yet - maybe duck type it into a promise.
  • cb(err, data) is annoying if there isn't ever an error. Why not just omit that parameter like it presently is?

I'm -1 on the .pipe(cat.string()) and .pipe(cat.buffer()) features but agree that getBody is too complex.

browserify v3 uses a typed array backed data structure for Buffer now, so you should just always get back a buffer or if the stream has been set to objectMode, you should get an array of objects. That would greatly simplify the getBody decoding.

Promises and generators would just add clutter to this relatively simple API.

should this library be able to concatenate streams - a bit like what https://github.com/felixge/node-combined-stream ?

if yes, I 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) ?

q2dg commented

What about streams2 implementation??

@jeromew this library is able to concatenate all the data within a single stream into a single datum (string, buffer, etc).

For the record, regarding error-catching ... (since I had to test this to find out)

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

function getSomething (cb) {
  var readable = getReadableStream();
  readable.on('error', end); // <--- handle upstream errors
  readable.pipe(concat(function(data){
    end(null, data);
  }));

  var ended = false;
  function end (error, data) {
    if (ended) return;
    ended = true; 
    cb(error, data); 
  }
}

Besides any upstream errors, there are no other errors to expect.