restify / clients

HttpClient, StringClient, and JsonClient extracted from restify
MIT License
57 stars 34 forks source link

fix(HttpClient): turn res stream to readable in HttpClient #175

Closed hekike closed 6 years ago

hekike commented 6 years ago

Turn res stream readable immediately to make it possible to consume later in inherited clients.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 87.024% when pulling 3231c45b59cf2366c7d8dd61317cd10a8daeb6a7 on fix/node-10-res-readable into 1edd6da9095a3754be8ed7cb7cc4bb3cecdef88b on master.

misterdjules commented 6 years ago

Great catch @hekike!

To give a bit more details: it seems the regression was introduced by https://github.com/nodejs/node/commit/cf5f9867ff3e700dfd72519e7bdeb701e254317f, which effectively makes adding a 'readable' listener stay in non-flowing mode even after a 'data' event listener is added to the same stream.

That change was released with Node.js v10.0.0. Before that change, adding a 'data' event listener to a stream would always make it "flow". It's a bit unfortunate that it doesn't seem to be listed anywhere in the list of backward incompatible changes.

Long term, ideally we would not mix both streaming modes if possible, and we would use read() instead of .on('data'. but that would potentially be a bit involved, and so for the near term your fix looks good to me.

misterdjules commented 6 years ago

It's a bit unfortunate that it doesn't seem to be listed anywhere in the list of backward incompatible changes.

My apologies, it is listed, since all major changes are listed in the changelog (look for SEMVER-MAJOR and readable).