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

Add loose-envify transform to package.json #519

Merged
merged 2 commits into from
Jan 2, 2017
Merged

Conversation

chrisvasz
Copy link
Contributor

We are building our application using Browserify. When including react-virtualized through Browserify, the process.env.NODE_ENV checks are not processed, causing Browserify to include a process shim in the bundle. This bloats the bundle size and reduces opportunity for minification since "development" !== "production" checks are no longer removable.

This PR tells Browserify to apply the loose-envify transform when react-virtualized is included in a Browserify bundle. After applying this change, process.env.NODE_ENV is correctly replaced during our build process.

Other libraries, such as Redux, use this method to provide process.env replacement too -- see here for the relevant commit on Redux.

@bvaughn
Copy link
Owner

bvaughn commented Dec 31, 2016

Hi @chrisvasz,

Thanks for the PR. 😄

I don't think loose-envify should not be added as a dependency for react-virtualized since (a) it isn't a runtime dependency and (b) not everyone uses Browserify. I think this package.json entry is just for projects that use Browserify to bundle JS, no?

Disclaimer: I'm not very familiar with Browserify.

Left the browserify transform in place for the loose-envify plugin.
@bvaughn
Copy link
Owner

bvaughn commented Jan 2, 2017

I'm going to commit the new browserify block by itself. If anything, it seems like loose-envify should be a devDependencies in an external project that actually uses Browserify. (react-virtualized uses Babel+Webpack so it doesn't seem appropriate to add the devDependencies here.) Please let me know if I've misunderstood anything. 😄

@bvaughn bvaughn merged commit 99766dd into bvaughn:master Jan 2, 2017
@chrisvasz
Copy link
Contributor Author

Thanks! This will save us about 7kb in production because of the propTypes removal and because Browserify will no longer include a process shim. Woohoo!

I do think loose-envify needs to be a dependency though. In my case, without the dependencies entry, this happens to work because I use other modules that require loose-envify and Browserify can find it in node_modules. If I remove the other projects that depend on loose-envify, my build breaks because Browserify can't find the loose-envify module.

From what I can tell, this is actually the reason Redux specifies loose-envify as a dependency (not a devDependency) also. Check out this conversation and this followup PR.

@bvaughn
Copy link
Owner

bvaughn commented Jan 3, 2017

Good call @chrisvasz. If you'd be willing to make a PR similar to reduxjs/redux/pull/1306 I'll accept it. Thanks again!

@chrisvasz
Copy link
Contributor Author

All over it. Thanks!

chrisvasz added a commit to chrisvasz/react-virtualized that referenced this pull request Jan 3, 2017
@bvaughn
Copy link
Owner

bvaughn commented Mar 9, 2018

Circling back to this because I found myself, again, wondering why we had to list loose-envify as a dependency instead of a dev dependency.

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.

2 participants