require('stre'+'am') doesn't do what I thought it did

[calvinmetcalf]

instead of only including stream if somebody else included it it apparently just doesn't include it in browserify vs node. This tells us a couple things

1. nobody is doing readable-stream instance of require('stream') in the browser
2. we could theoretically just replace this with a "browser":{"stream":false}
3. webpack is doing what I thought browserify did, but they are doing it wrong

[calvinmetcalf]

see webpack/webpack#4378

[mcollina]

Thanks @calvinmetcalf for taking this heads-on.
I'm 👍 on "browser": { "stream": false }.

Should we discuss this on the next wg meeting and release this ASAP?

[calvinmetcalf]

yeah

[parshap]

I've struggled with the require('st' + 'ream') calls while trying to get readable-stream to run in React Native. Here's what I've observed:

1. When using Browserify, require('st' + 'ream') throws an exception. This is because Browserify does not resolve 'st' + 'ream' at compile-time, so the module is not available at run-time. This is true even when another module has required stream before because the require function in Browserify is local to the module and only gives access to the dependencies resolved at compile-time.

2. When using React Native Packager, require('st' + 'ream') returns an empty object {}. This is because React Native Packager does resolve 'st' + 'ream' at compile-time and stream: false is set in package.json, which according to the package.json browser field spec, should cause the module to resolve to an empty object {}.

This results in readable-stream throwing an exception and crashing the app. My current solution is to patch readable-stream to remove require('st' + 'ream') calls in a postinstall hook.

Going forward, my suggestion is to:

1. Break instanceof compatibility with Node's builtin Stream. It wasn't working in the first place and no one complained.

2. If breaking this compatibility is not desired, set stream: false for both the browser and react-native fields in package.json. Call require('stream') and check that it returns Stream and not the empty object before using it.

I can submit a PR for this.

[mcollina]

I'm actually 👍 in removing instanceof  compatibility with Node's builtin Stream.

Can we do this without a semver-major?

[parshap]

I think this can be closed now since require('st' + 'ream') was removed in #269. Woohoo!