Skip to content

Conversation

@kumavis
Copy link
Contributor

@kumavis kumavis commented Dec 27, 2019

This is a breaking change (see https://github.com/nodejs/readable-stream#version-3xx)

Fixes #1880

@ljharb
Copy link
Member

ljharb commented Dec 27, 2019

Does this affect which engines a Browserify bundle can run on, or only which engines Browserify itself runs on?

@goto-bus-stop
Copy link
Member

I believe bundles using readable-stream@2 already worked on IE 11 and up because of the buffer dependency. However this needs to be done in stream-browserify, because readable-stream does not include the old streams1 API which is still in use in very popular packages like through.

@kumavis
Copy link
Contributor Author

kumavis commented Dec 28, 2019

Yeah I believe this involves dropping node versions <4 and perhaps some browsers as well. Slap it behind a major version number and lets let the past be the past 🚀

@ljharb
Copy link
Member

ljharb commented Dec 28, 2019

Please don’t drop older browsers. That choice is for app devs, not for a bundler to make.

@kumavis
Copy link
Contributor Author

kumavis commented Dec 28, 2019

@ljharb disagree! this is what semver is for. app devs retain the option to build with older versions of browserify. Currently browserify is failing in its mission to provide the node environment (subset) in a browser bundle by not shipping streams3

@kumavis
Copy link
Contributor Author

kumavis commented Dec 28, 2019

@ljharb @goto-bus-stop Errors in CI over at #1937 seem to imply that browserify's deps no longer support v0.8 v0.10

@ljharb
Copy link
Member

ljharb commented Dec 28, 2019

What semver is for is to communicate breakage; that in no way means we have to capriciously break people.

can you explain more about what is achieved by using streams3 that can’t be done with the current impl?

@ljharb
Copy link
Member

ljharb commented Dec 28, 2019

If deps have stopped supporting node versions without a major bump, we should peg them and file upstream bugs.

@kumavis
Copy link
Contributor Author

kumavis commented Dec 28, 2019

can you explain more about what is achieved by using streams3 that can’t be done with the current impl?

just one example of api not matching latest of node v10

readable-stream on  v2-no-util-mut [?] is 📦 v2.3.6 via ⬢ v10.17.0 
➜ echo 'console.log(require("stream").pipeline)' | node
[Function: pipeline]

readable-stream on  v2-no-util-mut [?] is 📦 v2.3.6 via ⬢ v10.17.0 
➜ echo 'console.log(require("stream").pipeline)' | browserify - | node
undefined

some behavioral differences documented here https://github.com/nodejs/readable-stream#version-3xx

If deps have stopped supporting node versions without a major bump, we should peg them and file upstream bugs.

I agree, just wanted to point this out

@ljharb
Copy link
Member

ljharb commented Dec 28, 2019

Is there any way to keep both and conditionally require it, or a simple way for browserify users to fall back to the older impl?

@kumavis
Copy link
Contributor Author

kumavis commented Dec 28, 2019

you can pass in builtins in options: browserify({ builtins: { xyz: './xyz.js' } })

@kumavis
Copy link
Contributor Author

kumavis commented Dec 28, 2019

that in no way means we have to capriciously break people.

I agree that there's no reason to break things for no reason, but supporting v0.8 will continue to be a barrier for changes going forward with browserify. like resolving the npm audit results.

found 62 vulnerabilities (1 low, 7 moderate, 53 high, 1 critical)

though I suppose there can be a distinction in platform support for the bundle we output vs the node version we run on

@kumavis
Copy link
Contributor Author

kumavis commented Dec 28, 2019

@goto-bus-stop @ljharb anyways, since there is going to be significant resistance to dropping support, would you mind adding a +1 to this readable-stream@2.x maintenance task where the main resistance is "its ancient"
nodejs/readable-stream#423

@kumavis
Copy link
Contributor Author

kumavis commented Dec 28, 2019

some scribbles #1940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

update readable-stream to v3

3 participants