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.

Sign up for GitHub

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

Jump to bottom

Prevent Shakapacker v7 deprecation messages #1592

Merged
merged 13 commits into from
Apr 3, 2024
Merged

Prevent Shakapacker v7 deprecation messages #1592

merged 13 commits into from
Apr 3, 2024

Conversation

ahangarha
Copy link
Contributor

@ahangarha ahangarha commented Jan 5, 2024 •
edited
Loading

Summary

This PR prevents displaying the Shakapcker 7+ deprecation message for:

  • webpacker_precompile?
  • webpacker:clean rake task

Pull Request checklist

  • Add/update test to cover these changes
  • Update documentation
  • Update CHANGELOG file

This change is Reviewable

justin808
justin808 requested changes Jan 6, 2024
View reviewed changes
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

see suggestion

justin808
justin808 reviewed Jan 6, 2024
View reviewed changes
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

Should there be any test?

@ahangarha ahangarha self-assigned this Jan 7, 2024
justin808
justin808 reviewed Jan 9, 2024
View reviewed changes
lib/react_on_rails/configuration.rb Outdated
@@ -266,6 +266,14 @@ def raise_missing_components_subdirectory

raise ReactOnRails::Error, msg
end

def shakapacker_precompile?
config = Webpacker.config
Copy link
Member

Choose a reason for hiding this comment

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

In the future, won't Webpacker be unavailable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.
But still, we need to provide support for the old spelling.

As soon as we force the upgrade to Shakapacker 7+, we need to have support for webpacker spelling.

Copy link
Member

Choose a reason for hiding this comment

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

At some point, we release an update to Shakapacker.

Somebody has not yet updated React on Rails.

config = Webpacker.config crashes.

Check that the constant Webpacker exists before calling it.

Other code spots might have the same issue. Check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's fix this issue in another dedicated PR.

I think at some point, we should drop support Shakapacker 6. I think it is important to push projects to move to use the new API for shakapacker and version 7 was the transitional version for that migration.

@ahangarha ahangarha force-pushed the prevent-webpacker-precompile-deprecation-message branch from 546982c to b4a324e Compare January 9, 2024 14:06
@ahangarha ahangarha changed the title Prevent Webpacker precompile deprecation message Prevent Shakapacker v7 deprecation messages Jan 9, 2024
@ahangarha ahangarha changed the title Prevent Shakapacker v7 deprecation messages WIP - Prevent Shakapacker v7 deprecation messages Jan 9, 2024
@justin808
Copy link
Member

Please fix conflicts and see my comments.

@justin808
Copy link
Member

@ahangarha please finish up so I can merge.

@ahangarha ahangarha force-pushed the prevent-webpacker-precompile-deprecation-message branch 3 times, most recently from 913b63b to dbbc7ff Compare January 21, 2024 17:45
@ahangarha ahangarha changed the title WIP - Prevent Shakapacker v7 deprecation messages Prevent Shakapacker v7 deprecation messages Jan 21, 2024
justin808
justin808 reviewed Jan 22, 2024
View reviewed changes
Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

See comment in #1598. Otherwise this is OK.

@ahangarha ahangarha force-pushed the prevent-webpacker-precompile-deprecation-message branch from ad2422f to 2d91229 Compare January 22, 2024 15:01
ahangarha
ahangarha commented Jan 22, 2024
View reviewed changes
@ahangarha ahangarha mentioned this pull request Jan 22, 2024
Merged
3 tasks
@ahangarha ahangarha force-pushed the prevent-webpacker-precompile-deprecation-message branch from 2d91229 to cfcc3e1 Compare January 24, 2024 16:24
@ahangarha ahangarha requested a review from justin808 January 24, 2024 16:45
@ahangarha ahangarha force-pushed the prevent-webpacker-precompile-deprecation-message branch from cfcc3e1 to 19762e4 Compare March 6, 2024 08:17
justin808
justin808 reviewed Mar 12, 2024
View reviewed changes
lib/react_on_rails/configuration.rb
def shakapacker_precompile?
return Webpacker.config.webpacker_precompile? if ReactOnRails::WebpackerUtils.using_shakapacker_6?

Webpacker.config.shakapacker_precompile?
Copy link
Member

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

Shakapacker.config.shakapacker_precompile?

Copy link
Contributor Author

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.

lib/react_on_rails/webpacker_utils.rb
def self.using_shakapacker_6?
shakapacker_major_version = shakapacker_version_as_array[0]

shakapacker_major_version == 6
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Can we be sure that nobody uses that method?

spec/react_on_rails/generators/dev_tests_generator_spec.rb
@@ -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"')
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, such changes are in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. But it was needed to pass Rubocop.

@justin808 justin808 merged commit f47e95e into master Apr 3, 2024
19 checks passed
@justin808 justin808 deleted the prevent-webpacker-precompile-deprecation-message branch April 3, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justin808 justin808 justin808 requested changes

@Judahmeek Judahmeek Awaiting requested review from Judahmeek

Assignees

@ahangarha ahangarha

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahangarha @justin808 @Judahmeek