browserify / http-browserify

node's http module, but for the browser
MIT License
245 stars 110 forks source link

Response inherits from Stream instead of Stream.Readable (requiring request module browserify workaround) #81

Open deathcap opened 9 years ago

deathcap commented 9 years ago

On node.js, the HTTP response object (which is named IncomingMessage in node.js) inherits from Stream.Readable:

util.inherits(IncomingMessage, Stream.Readable);

but in http-browserify, it only inherits from Stream:

util.inherits(Response, Stream);

This means the full stream API is not available as it is in Node, including the .resume() method. I believe this is the reason for this hack in the request module:

  } else if (response.resume) {
    // response.resume should be defined, but check anyway before calling.
    // Workaround for browserify.
    response.resume()
  }

to improve compatibility would it be possible for http-browserify Response to inherit from Stream.Readable? (are there any other changes needed than changing the utils.inherits call?)

edit: Stream is old-style (pre-0.10); this amounts to converting http-browserify to Streams2

goloroden commented 9 years ago

+1

mattwagl commented 9 years ago

We (@goloroden and me) have written two small modules to stream JSON lines from a server to a client. Could be a very simple alternative to things like socket.io.

The client run’s great inside node and as we’re big fans of browserify we’d like to make it work inside the browser as well. But we're currently facing some compatibility problems that seem to be related to this issue.

When a client disconnects we’re using res.resume() to cleanup the response stream. However this fails when using browserify as Response currently doesn't inherit from Stream.Readable.

Do you have any plans to upgrade this module to the streams2 API? Maybe we can help to get this done. Any help or tips to get this issue resolved would be greatly appreciated.

P.S.: Thanks for creating this awesome browserify ecosystem!

goloroden commented 9 years ago

+1 :-)

goloroden commented 9 years ago

For anybody having the same issues, @mattwagl and I have created a drop-in replacement for http-browserify that uses the streams2 API: http-browserify-2