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

NODE_ENV needs to default to 'development' if not set #183

Closed
justin808 opened this issue Aug 31, 2022 · 5 comments · Fixed by #185
Closed

NODE_ENV needs to default to 'development' if not set #183

justin808 opened this issue Aug 31, 2022 · 5 comments · Fixed by #185
Assignees
Labels

Comments

@justin808
Copy link
Member

Problem

NODE_ENV == test needs to be saved for JS only code, such as running Jest and Storybook, and NEVER for rspec or other integration tests.

Broken originally, like this:

https://github.com/rails/webpacker/blame/93386ed7e85928b38b46f3f3dd259ae3a9e59090/lib/install/bin/webpack

Fixed:

rails/webpacker#1359

https://github.com/rails/webpacker/blame/33b6c05ddb615ffa1a860c274372e21876d71c01/lib/install/bin/webpack

ENV["NODE_ENV"]  ||= "development"

But if one forgets to set NODE_ENV for production builds, the JS code is not optimized!

This change broke the functionality:

https://github.com/shakacode/shakapacker/blame/master/lib/install/bin/webpacker#L9

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

The Suggested Fix

  1. Reset the default back to NODE_ENV == development in the bin/webpacker and bin/webpacker-dev-server files
  2. Need to ensure that NODE_ENV == production
# don't overwrite, so use ||=
ENV['NODE_ENV'] ||= (ENV['RAILS_ENV'] == production) ? 'production' : 'development'

But since this code is only updated when one makes a new install, let's move the change here:

from the binstub

Dir.chdir(APP_ROOT) do
  Webpacker::WebpackRunner.run(ARGV)
end

And update this file:

https://github.com/shakacode/shakapacker/blob/master/lib/webpacker/runner.rb

And then, the upgrade instructions suggest removing the setting of the NODE_ENV from the bin stubs.

@justin808
Copy link
Member Author

@tomdracz @Judahmeek @alexeyr-ci1 please review.

@tomdracz
Copy link
Collaborator

tomdracz commented Sep 1, 2022

Makes sense to me, looks like a sensible change!

@alexeyr-ci1
Copy link
Collaborator

Makes sense to me, looks like a sensible change!

Agreed.

@alexeyr-ci1
Copy link
Collaborator

@justin808

And then, the upgrade instructions suggest removing the setting of the NODE_ENV from the bin stubs.

Some people may be using the stubs directly. My suggestion would be to change them as well instead of removing.

@mage1711 mage1711 linked a pull request Sep 5, 2022 that will close this issue
@justin808
Copy link
Member Author

Some people may be using the stubs directly. My suggestion would be to change them as well instead of removing.

Yes, remove from the binstubs installed, and be sure to mention this in the CHANGELOG.

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

4 participants