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

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

Open
deathcap opened this issue Feb 11, 2015 · 4 comments

Comments

@deathcap
Copy link

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

+1

@mattwagl
Copy link

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

+1 :-)

@goloroden
Copy link

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

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

3 participants