-
-
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
Prevent Shakapacker v7 deprecation messages #1592
Changes from all commits
67fac2c
f9a81b8
1ce8095
3b7dd0b
a77766c
30bce62
2ebe31e
f1c5e1b
dae3966
9968df1
5bf8e78
19762e4
b184619
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,5 +141,11 @@ def self.raise_shakapacker_not_installed | |
def self.semver_to_string(ary) | ||
"#{ary[0]}.#{ary[1]}.#{ary[2]}" | ||
end | ||
|
||
def self.using_shakapacker_6? | ||
shakapacker_major_version = shakapacker_version_as_array[0] | ||
|
||
shakapacker_major_version == 6 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shakapacker_version[:major] == 6 isn't that nicer? you can change the shakapacker_version method very easily. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, if it is okay to modify that method, your suggestion is much much better. |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# frozen_string_literal: true | ||
|
||
require "rake" | ||
|
||
require "rails_helper" | ||
|
||
describe "rake assets:precompile task" do | ||
it "doesn't show deprecation message for using webpacker:clean task" do | ||
allow(ENV).to receive(:[]).with(anything).and_call_original | ||
allow(ENV).to receive(:[]).with("SHAKAPACKER_PRECOMPILE").and_return("false") | ||
|
||
Rails.application.load_tasks | ||
|
||
ReactOnRails.configure do |config| | ||
config.build_production_command = "RAILS_ENV=production NODE_ENV=production bin/shakapacker" | ||
end | ||
|
||
expect do | ||
system "bundle install && yarn install && yarn run build:test" | ||
Rake::Task["assets:precompile"].execute | ||
end.not_to output(/Consider using `rake shakapacker:clean`/).to_stdout | ||
|
||
Rake::Task.clear | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,16 +25,16 @@ | |
|
||
it "changes package.json to use local react-on-rails version of module" do | ||
assert_file("package.json") do |contents| | ||
assert_match('"react-on-rails"', contents) | ||
assert_match('"postinstall"', contents) | ||
expect(contents).to match('"react-on-rails"') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally, such changes are in a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. But it was needed to pass Rubocop. |
||
expect(contents).to match('"postinstall"') | ||
end | ||
end | ||
|
||
it "adds test-related gems to Gemfile" do | ||
assert_file("Gemfile") do |contents| | ||
assert_match("gem \"rspec-rails\", group: :test", contents) | ||
assert_match("gem \"coveralls\", require: false", contents) | ||
assert_match("gem \"chromedriver-helper\", group: :test", contents) | ||
expect(contents).to match("gem \"rspec-rails\", group: :test") | ||
expect(contents).to match("gem \"coveralls\", require: false") | ||
expect(contents).to match("gem \"chromedriver-helper\", group: :test") | ||
end | ||
end | ||
end | ||
|
@@ -49,7 +49,7 @@ | |
|
||
it "adds prerender for examples with example-server-rendering" do | ||
assert_file("app/views/hello_world/index.html.erb") do |contents| | ||
assert_match("prerender: true", contents) | ||
expect(contents).to match("prerender: true") | ||
end | ||
end | ||
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.
@ahangarha @Judahmeek should this be
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.
I think we should do this when we drop support for Webpacker/Shakapacker 6.
This way, we still know we are supporting the old interface. Otherwise, changing the name is not a big issue and can be done in a separate PR.