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

v2 ideas #15

Open
jonathanong opened this issue Dec 1, 2013 · 4 comments
Open

v2 ideas #15

jonathanong opened this issue Dec 1, 2013 · 4 comments

Comments

@jonathanong
Copy link

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 How to handle errors without err callback argument #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 (Reemit chunks and events so we can pipe concat-streams! #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.
@ghost
Copy link

ghost commented Dec 24, 2013

  • 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.

@jeromew
Copy link

jeromew commented Jan 13, 2014

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
Copy link

q2dg commented Oct 11, 2014

What about streams2 implementation??

@zenflow
Copy link

zenflow commented Jun 29, 2015

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants