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

feat(libs): replace http-browserify with stream-http. #41

Merged
merged 1 commit into from
Nov 7, 2016

Conversation

terinjokes
Copy link
Contributor

Fixes #38.

@TheLarkInn
Copy link
Member

Have you run this updated package against webpack master? What concerns me is that this is a dep of webpack, we should probably someway verify it works in core. If we can verify that, I'm sure we can merge this.

@terinjokes
Copy link
Contributor Author

@TheLarkInn If it's any consolation browserify switched to stream-http several versions ago, and it's a much more accurate implementation of the Node.js builtin. http-browserify, on the other hand, is unmaintained.

Is there another test I should be running on master other than "did it get bundled"?

@TheLarkInn
Copy link
Member

Welp I think one would be npm link the dep to webpack on master, then run tests locally. I'd list that results here

@terinjokes
Copy link
Contributor Author

@TheLarkInn Let me know if there are reasonable tests to contribute, either to this repo or to webpack. I didn't see any tests in webpack for the builtins.

@RomanHotsiy
Copy link

RomanHotsiy commented Aug 28, 2016

@terinjokes I believe https-browserify should be replaced with streem-http too.
Check out this issue substack/https-browserify#6. (Although it is closed, the issue itself is not resolved)

@terinjokes
Copy link
Contributor Author

I commented on the above ticket.

@feross
Copy link

feross commented Sep 4, 2016

cc @jhiesey

@connor4312
Copy link

Getting this in would be fantastic! stream-http is much more featureful and has greater parity with the server-side version.

@dandv
Copy link

dandv commented Nov 2, 2016

Any chance of merging this in?

@jhnns
Copy link
Member

jhnns commented Nov 3, 2016

Thx @terinjokes. This looks good. Could you sign the CLA?

Sorry for taking so long, but switching these libraries is kind of risky because they provide an incredible big API surface to the user. Most of developers don't know this. However, since browserify is using it too, it's very safe – and it provides a consistent experience for webpack- and browserify-users.

@jhnns
Copy link
Member

jhnns commented Nov 3, 2016

The PR was against node-libs-browser master which is 1.0.0. After merging this, I will try to backport most of the updates. @sokra told me, there was a breaking change in the buffer module or somewhere there, that's why he decided to release a new version but he can't remember it for sure.

@terinjokes
Copy link
Contributor Author

@jhnns Per the CLA, can you first add a LICENSE file? The license listed in package.json is "MIT".

jhnns added a commit that referenced this pull request Nov 4, 2016
- Necessary for CLA, see #41
- Without current year since it is not necessary, see facebook/react@bef45b0
@jhnns
Copy link
Member

jhnns commented Nov 4, 2016

Haha, good catch, thx. I added the MIT license.

@jhnns jhnns merged commit 1fe30d4 into webpack:master Nov 7, 2016
@jhnns
Copy link
Member

jhnns commented Nov 7, 2016

Thx @terinjokes

We will probably update node-libs-browser also in webpack v1, see #45

@feross
Copy link

feross commented Nov 10, 2016

stream-http is still not being used with latest webpack because this isn't published to npm yet.

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

Successfully merging this pull request may close these issues.

None yet

7 participants