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

Necessary to set the NODE_ENV in install files like config/webpack/production.js? #2463

Closed
justin808 opened this issue Feb 15, 2020 · 2 comments

Comments

@justin808
Copy link
Contributor

All the install files have this:

process.env.NODE_ENV = process.env.NODE_ENV || 'development'
const environment = require('./environment')
module.exports = environment.toWebpackConfig()

Why do we need the first line? From what I can tell, the files are picked based on the NODE_ENV, so it seems impossible that this is out of sync. Furthermore, we could set the NODE_ENV if not set before running the file.

Here is where the NODE_ENV is used to pick what file is used, so I'm a bit puzzled as to how the NODE_ENV might not be set.

https://github.com/rails/webpacker/blob/master/lib/webpacker/runner.rb#L14

   @webpack_config        = File.join(@app_path, "config/webpack/#{ENV["NODE_ENV"]}.js")

If needed, we could set the process.env.NODE_ENV since these files are all run from the:

https://github.com/rails/webpacker/blob/master/lib/webpacker/webpack_runner.rb#L23

Kernel.exec env, *cmd

I left a code comment where this was introduced.

e616184#r37306278

@justin808 justin808 changed the title Necessary to set the NODE_ENV Necessary to set the NODE_ENV in install files like config/webpack/production.js? Feb 15, 2020
@jakeNiemiec
Copy link
Member

And the cmd value is based on the NODE_ENV to pick what file is used, so I'm a bit puzzled as to how the NODE_ENV might not be set.

I think you can call webpack without rails, but I haven't tested this recently. That line most likely comes from https://glebbahmutov.com/blog/do-not-use-node-env-for-staging/
Also keep this in mind: #1212 (comment)

I do agree that there could be room for tightening, but I haven't really done any dev work in this part of the codebase. It's kinda always "just worked". The only thing I would look out for is to ensure that users can still override the NODE_ENV from both the cmd line (running bins) & environment.js. (I'm not sure which would take precedence 🤔)

@justin808
Copy link
Contributor Author

While I don't think the first line is necessary:

process.env.NODE_ENV = process.env.NODE_ENV || 'development'

It probably does no harm to keep it to ensure that it's set in case one will use a specific webpack config directly rather than calling the Rails bin/webpack.

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

No branches or pull requests

2 participants