envify in peerDependencies complicates use with node

[benatkin]

The package uses peerDependencies in package.json for envify. Why not use dependencies instead? The adoption of peerDependencies is low and it's being debated whether it should be kept in node.

browserify-cdn is a tool which requirebin uses. requirebin is a nifty tool for building modular examples and sharing them. I would like to use it with react, but browserify-cdn doesn't work well with peerDependencies. It would be extra work to make it work with them, and anyone building a similar tool might have to do the extra work as well. Because of this I would very much like node packages that are designed to be used client-side to not rely on peerDependencies.

[sophiebits]

I think we can move it to dependencies, though it's a little silly in either case to require people who might not be using browserify to install it. @petehunt Any objections?

[benatkin]

It doesn't seem like it's being used in the code that makes it on to npm. I searched with ag and all occurrences seem to be inside of node_modules/envify. If it's used in a build script that doesn't make it on to npm, it should be in devDependencies instead.

BTW I'm glad to know about envify, it has made me aware of security issues with credentials being stored in environment variables.

[sophiebits]

It's used as a browserify transform when building a bundle so needs to be required then. It's not used before npm so doesn't belong in devDependencies.

[benatkin]

Yep, just searched the source (as opposed to what npm installs) and it's in grunt/config/browserify.js.

[sophiebits]

No, that's for the non-npm (browser) builds. envify is referenced in the browserify transform array here:

https://github.com/facebook/react/blob/master/npm-react/package.json

[benatkin]

I see. It appears the envify transform might be a no-op (more info below). Perhaps both the dependency and the transform should be removed, but envify should keep being used by grunt.

I created a file index.js with:

var React = require('react')

I ran npm install react and browserify index.js -o with.js.

I then removed the transform in node_modules/react/package.json and ran browserify index.js -o without.js.

I ran diff with.js without.js and it shows no output. I then tried making a change to one of the source files in node_modules/react to make sure browserify used that copy of react, generated a new bundle, and the change registered in the diff.

It appears this is because the JavaScript in the npm package doesn't reference process.env at all.

[sophiebits]

I see process.env.NODE_ENV many times in the resulting file, but presumably this means envify is indeed not working properly.

[benatkin]

I fiddled with it and found that it only works if process.env.NODE_ENV is set. I set process.env.NODE_ENV to 'development' and got a different result.

However, I think that it's better to leave envify up to the developer making a bundle with browserify, rather than including the transform. One good place to do this is after the concatenation but before the minification. This is what [uglifyify](https://github.com/hughsk/uglifyify#motivationusage suggests in its README). So I think the change you made is a good permanent solution to this issue.