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

Change NODE_ENV defaults #185

Merged

Conversation

mage1711
Copy link
Member

@mage1711 mage1711 commented Sep 5, 2022

changed NODE_ENV defaults from

ENV["NODE_ENV"]  ||= ENV["RAILS_ENV"]

to

ENV['NODE_ENV'] ||= (ENV['RAILS_ENV'] == production) ? 'production' : 'development'

@mage1711 mage1711 linked an issue Sep 5, 2022 that may be closed by this pull request
@mage1711 mage1711 self-assigned this Sep 5, 2022
@mage1711 mage1711 added the bug label Sep 5, 2022
README.md Outdated
@@ -350,6 +350,7 @@ Note, if you are using server-side rendering of JavaScript with dynamic code-spl
### Development

Webpacker ships with two binstubs: `./bin/webpacker` and `./bin/webpacker-dev-server`. Both are thin wrappers around the standard `webpack.js` and `webpack-dev-server.js` executables to ensure that the right configuration files and environmental variables are loaded based on your environment.
The setting of the NODE_ENV should be removed from the binstubs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go to the changelog, not here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go to the changelog, not here.

I added it to the changelog also https://github.com/shakacode/shakapacker/pull/185/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR10

and I think we should keep this line or move it somewhere else because the old binstubs will still have the old NODE_ENV defaults if a new install isn't done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change, @mage1711:

Older Shakapacker installations had the setting of a missing NODE_ENV in the binstubs as the default per RAILS_ENV is no longer set there.

@mage1711 mage1711 marked this pull request as ready for review September 6, 2022 17:21
README.md Outdated
@@ -350,6 +350,7 @@ Note, if you are using server-side rendering of JavaScript with dynamic code-spl
### Development

Webpacker ships with two binstubs: `./bin/webpacker` and `./bin/webpacker-dev-server`. Both are thin wrappers around the standard `webpack.js` and `webpack-dev-server.js` executables to ensure that the right configuration files and environmental variables are loaded based on your environment.
The setting of the NODE_ENV should be removed from the binstubs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change, @mage1711:

Older Shakapacker installations had the setting of a missing NODE_ENV in the binstubs as the default per RAILS_ENV is no longer set there.

@mage1711 mage1711 requested a review from justin808 September 7, 2022 09:38
@justin808 justin808 merged commit a0d34a2 into master Sep 8, 2022
@justin808 justin808 deleted the 183-node_env-needs-to-default-to-development-if-not-set branch September 8, 2022 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NODE_ENV needs to default to 'development' if not set
3 participants