-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Replace http-browserify with stream-http #1327
Conversation
I am certainly willing to test it with some important modules that require('http') in browserify if anyone can suggest some good test cases. |
cc @feross |
This looks good from a once-over of the stream-http source. It seems to work with the var request = require('request');
var r = request(location.protocol + '//' + location.host + '/x.txt');
var chunks = 0;
r.on('data', function () { chunks ++ });
r.on('end', function () { console.log('chunks=', chunks) });
I also get this warning in chrome 41:
|
It looks like the double 'end' is coming from this (arguably wrong and no longer necessary) logic in request: https://github.com/request/request/blob/master/request.js#L931 However, I also don't think stream-http should be emitting 'close', since there is no real underlying socket and node (at least often?) doesn't emit it even if the connection definitely does close. These should be fixed in v1.1.1 |
By the way, on the subject of warnings, I'd love to eliminate these in chrome:
I don't have any idea how to do this, however, while still doing the feature detection I'm trying to do. |
@jhiesey The issue with the unhidable warning should probably be opened up as a bug on chromium: https://code.google.com/p/chromium/issues/list |
@jhiesey If node emits 'close', which the docs say it does, then I think browserify should do the same. Users could be listening for that event. (It's weird that you're seeing node sometimes not emit the 'close' event.) Also, I think the bug with |
I would also implement |
stream-http uses new-style streams to provide true streaming or pseudo-streaming in as many browsers as possible. It uses the new fetch api with native browser streams where available (currently only chrome supports this combination) and various browser-specific xhr extensions to make binary-safe streaming work where possible. It is tested and working with at least basic functionality back to IE8.
OK, I think I've addressed all of these issues. |
If there are no other objections, I think this is ready to merge. To be safe, I think this should be a major version bump. We did the same when we switched the buffer implementation in v3. Let's wait a few days to give other people a chance to weigh in on this PR? |
stream-http uses new-style streams to provide true streaming or
pseudo-streaming in as many browsers as possible.
It uses the new fetch api with native browser streams where available
(currently only chrome supports this combination) and various
browser-specific xhr extensions to make binary-safe streaming
work where possible.
It is tested and working with at least basic functionality
back to IE8.