tomas / needle

Nimble, streamable HTTP client for Node.js. With proxy, iconv, cookie, deflate & multipart support.
https://www.npmjs.com/package/needle
MIT License
1.63k stars 236 forks source link

Emit 'done' event on writable streams Needle's response is being piped to #373

Closed tomas closed 3 years ago

tomas commented 3 years ago

This lets us use a consistent API regardless listening to events on the response stream itself or to destination/writable streams.

This is one of the examples in the readme:

needle
  .post('https://my.server.com/foo', data, { multipart: true })
  .on('readable', function() { /* eat your chunks */ })
  .on('done', function(err) {
    console.log('Ready-o!');
  })

However if instead of processing the chunks manually you wanted to pipe to another stream, then you'd be attaching the on('done') listener to the target stream:

needle
  .post('https://my.server.com/foo', data, { multipart: true })
  .pipe(someWritableStream)
  .on('done', function(err) {
    // this would never fire, because it's Needle's response 
    // that emits the 'done' event, not the writableStream returned by the pipe() function
  })

This PR ensures that both Needle's response and its target streams, when piped, all emit the 'done' event, providing a more consistent way of piping streams.

On a related note, please take a look at #372 for more info regarding error handling on streams. The gist of it is that you should use stream.pipeline instead of pipe, when possible.