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

Revert of production env enforcement #2341

Merged
merged 7 commits into from Nov 7, 2019

Conversation

@coding-bunny
Copy link
Contributor

coding-bunny commented Oct 30, 2019

Description

The change made in d8dd446 forces the production environment on every compile, rake task and usage of the webpacker gem, ignoring the environment variables used by the system and/or developer.

  1. The tool does not get to decide how it's running, it needs to respect the ENV.
  2. This breaks CI systems that have specific configurations
  3. It steps away from convention over configuration.

By retaining the original code, and defaulting to the production environment, it's possible for people to compile their assets and dependencies under their requirements, ensuring no breaking changes in the systems. Proper documentation can/should be added to explain the usage of the NODE_ENV and how it affects packages for people not familiar with Node and NPM, but this change has too much breaking potential.

Resolves #2340

Undoing the commit that forces the production environment. The code has it as default when not defined, this is sufficient, but it should respect the user settings!

Resolves #2340
@coding-bunny coding-bunny changed the title Revert of d8dd4460144e63a9120290631c492aba3cbea332 Revert of production env enforcement Oct 30, 2019
use proper indentation
Removed call to `with_node_env` and assume production by default.
updated failing tests to reflect changes made.
inject the ENV over command line
trying to fix RuboCop
@jakeNiemiec

This comment has been minimized.

Copy link
Member

jakeNiemiec commented Oct 31, 2019

@gauravtiwari Do you have any strong opinions on this? I worry that this is a footgun.

@rossta

This comment has been minimized.

Copy link
Contributor

rossta commented Nov 7, 2019

To add another take here, what about instead, something like the following?

Webpacker.with_node_env(ENV.fetch("NODE_ENV", "production")) do
# ...

I believe this would maintain the spirit of the change in question, which was to remove global env pollution, i.e. ENV["NODE_ENV"] ||= 'production' to fix #1374, while also satisfying the desire stated here: to allow the override of the default behavior.

@coding-bunny

This comment has been minimized.

Copy link
Contributor Author

coding-bunny commented Nov 7, 2019

Implemented your change @rossta
This makes the README actually correct again, and allows overwriting/using different environments.
Default is production, which allows the suggestions from #1374 to work as well by not setting the NODE_ENV

@gauravtiwari gauravtiwari merged commit 4c79d22 into rails:master Nov 7, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.