-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Move webpacker task adjustment logic to Configuration class method #1415
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion
lib/tasks/assets.rake, line 12 at r1 (raw file):
skip_react_on_rails_precompile = %w[no false n f].include?(ENV["REACT_ON_RAILS_PRECOMPILE"]) if !skip_react_on_rails_precompile && ReactOnRails.configuration.build_production_command.present?
I could not figure out how to access the Rails.application from inside this file, so my solution was to remove this check, always set ENV["WEBPACKER_PRECOMPILE"] = "false"
, & then invoke the webpacker tasks if build_production_command
isn't defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion
lib/tasks/assets.rake, line 12 at r1 (raw file):
Previously, Judahmeek (Judah Meek) wrote…
I could not figure out how to access the Rails.application from inside this file, so my solution was to remove this check, always set
ENV["WEBPACKER_PRECOMPILE"] = "false"
, & then invoke the webpacker tasks ifbuild_production_command
isn't defined.
if ENV["WEBPACKER_PRECOMPILE"] = "false"
, doesn't that mean that webpacker:clobber
will be skipped as well?
bc28234
to
47fba9a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions
spec/dummy/config/initializers/react_on_rails.rb, line 23 at r2 (raw file):
# config.build_test_command = "yarn run build:test" # config.build_production_command = "yarn run build:prod"
Should I uncomment this & then add a spec that confirms that ENV["WEBPACKER_PRECOMPILE"]
is false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r3, all commit messages.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @Judahmeek and @justin808)
spec/dummy/config/initializers/react_on_rails.rb, line 23 at r2 (raw file):
Previously, Judahmeek (Judah Meek) wrote…
Should I uncomment this & then add a spec that confirms that
ENV["WEBPACKER_PRECOMPILE"]
isfalse
?
Yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 6 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Judahmeek and @justin808)
spec/dummy/config/initializers/react_on_rails.rb, line 23 at r5 (raw file):
# config.build_test_command = "yarn run build:test" config.build_production_command = "echo 'ran ReactOnRails config.build_production_command'"
But this example then doesn't work...
@gscarv13 made this so it is deployed to heroku.
Let's keep as commented, with bin/webpack
as the command that it runs.
spec/dummy/spec/initialized_configuration_spec.rb, line 7 at r5 (raw file):
describe "ReactOnRails initializer" do it "changes ENV[\"WEBPACKER_PRECOMPILE\"] to \"false\" because config.build_production_command is defined" do expect(ENV["WEBPACKER_PRECOMPILE"]).to eq("false")
we can move this to a unit test for config, see below
spec/react_on_rails/configuration_spec.rb, line 83 at r5 (raw file):
end ENV["WEBPACKER_PRECOMPILE"] = "false"
this test doesn't make sense
let's check both cases of set and not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Judahmeek)
lib/tasks/assets.rake, line 12 at r1 (raw file):
Previously, Judahmeek (Judah Meek) wrote…
if
ENV["WEBPACKER_PRECOMPILE"] = "false"
, doesn't that mean thatwebpacker:clobber
will be skipped as well?
yes, that's OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @gscarv13 and @justin808)
spec/dummy/config/initializers/react_on_rails.rb, line 23 at r5 (raw file):
Previously, justin808 (Justin Gordon) wrote…
But this example then doesn't work...
@gscarv13 made this so it is deployed to heroku.
Let's keep as commented, with
bin/webpack
as the command that it runs.
Done.
spec/react_on_rails/configuration_spec.rb, line 83 at r5 (raw file):
Previously, justin808 (Justin Gordon) wrote…
this test doesn't make sense
let's check both cases of set and not set
My bad. Copied the wrong line there.
My latest commit does check both set & unset.
The first expect
is while the build_production_command
is unset & the second expect
confirms that setting build_production_command
results in ENV["WEBPACKER_PRECOMPILE"] being set to "false"
spec/dummy/spec/initialized_configuration_spec.rb, line 7 at r5 (raw file):
Previously, justin808 (Justin Gordon) wrote…
we can move this to a unit test for config, see below
Done.
end | ||
|
||
expect(ENV["WEBPACKER_PRECOMPILE"]).to eq("false") | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the lines above 79-80 with nil and check that the value for the ENV is reset.
2053d06
to
6559676
Compare
lib/react_on_rails/configuration.rb
Outdated
def adjust_precompile_task | ||
skip_react_on_rails_precompile = %w[no false n f].include?(ENV["REACT_ON_RAILS_PRECOMPILE"]) | ||
|
||
return unless !skip_react_on_rails_precompile && build_production_command.present? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return if skip_react_on_rails_precompile || build_production_command.blank?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r6, 1 of 1 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @Judahmeek)
The only way I found to actually test the logic of
assets.rake
with an un-initialized Rails application was by usingbundle exec irb
This change is