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

envify in peerDependencies complicates use with node #1482

Closed
benatkin opened this issue May 5, 2014 · 8 comments
Closed

envify in peerDependencies complicates use with node #1482

benatkin opened this issue May 5, 2014 · 8 comments

Comments

@benatkin
Copy link

benatkin commented May 5, 2014

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
Copy link
Collaborator

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
Copy link
Author

benatkin commented May 5, 2014

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
Copy link
Collaborator

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
Copy link
Author

benatkin commented May 5, 2014

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

@sophiebits
Copy link
Collaborator

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
Copy link
Author

benatkin commented May 5, 2014

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
Copy link
Collaborator

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

sophiebits added a commit to sophiebits/react that referenced this issue May 5, 2014
Fixes facebook#1482.

It doesn't seem like this is actually doing anything and it's causing problems for people, so remove it.
@benatkin
Copy link
Author

benatkin commented May 5, 2014

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.

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 a pull request may close this issue.

2 participants