-
-
Notifications
You must be signed in to change notification settings - Fork 638
Fix Shakapacker version requirement from 8.2.0 to 8.0.0 for basic compatibility #1801
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
Changes from 8 commits
a3c3391
233b789
339451a
0a566cd
759e360
07e68db
7fdc7b4
30e0e6d
37e54a1
68f4baf
1e2f03c
c5ae0f9
35784e9
bc73e88
a575302
943b843
00d2670
b672d92
27eda87
70b4c7d
5b3e5ff
9db5190
67210ef
da9420e
e038121
8d03cc6
5d865c0
437cec8
4b932ad
9ca52d1
8a33f7d
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 | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,33 +2,12 @@ | |||||||||||||||||||
|
|
||||||||||||||||||||
| module ReactOnRails | ||||||||||||||||||||
| module PackerUtils | ||||||||||||||||||||
| def self.using_packer? | ||||||||||||||||||||
| using_shakapacker_const? | ||||||||||||||||||||
| end | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def self.using_shakapacker_const? | ||||||||||||||||||||
| return @using_shakapacker_const if defined?(@using_shakapacker_const) | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @using_shakapacker_const = ReactOnRails::Utils.gem_available?("shakapacker") && | ||||||||||||||||||||
| shakapacker_version_requirement_met?("8.2.0") | ||||||||||||||||||||
| end | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def self.packer_type | ||||||||||||||||||||
| return "shakapacker" if using_shakapacker_const? | ||||||||||||||||||||
|
|
||||||||||||||||||||
| nil | ||||||||||||||||||||
| end | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def self.packer | ||||||||||||||||||||
| return nil unless using_packer? | ||||||||||||||||||||
|
|
||||||||||||||||||||
| require "shakapacker" | ||||||||||||||||||||
| ::Shakapacker | ||||||||||||||||||||
| end | ||||||||||||||||||||
|
|
||||||||||||||||||||
| def self.dev_server_running? | ||||||||||||||||||||
| return false unless using_packer? | ||||||||||||||||||||
|
|
||||||||||||||||||||
| packer.dev_server.running? | ||||||||||||||||||||
| end | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -38,7 +17,6 @@ def self.dev_server_url | |||||||||||||||||||
|
|
||||||||||||||||||||
| def self.shakapacker_version | ||||||||||||||||||||
| return @shakapacker_version if defined?(@shakapacker_version) | ||||||||||||||||||||
| return nil unless ReactOnRails::Utils.gem_available?("shakapacker") | ||||||||||||||||||||
|
|
||||||||||||||||||||
| @shakapacker_version = Gem.loaded_specs["shakapacker"].version.to_s | ||||||||||||||||||||
|
||||||||||||||||||||
| @shakapacker_version = Gem.loaded_specs["shakapacker"].version.to_s | |
| spec = Gem.loaded_specs["shakapacker"] | |
| unless spec | |
| raise ReactOnRails::Error, "Shakapacker gem is required but not loaded. Please add 'shakapacker' to your Gemfile and run bundle install." | |
| end | |
| @shakapacker_version = spec.version.to_s |
Copilot
AI
Sep 22, 2025
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.
The method doesn't check if Shakapacker is available before calling shakapacker_version_requirement_met?. If Shakapacker is not installed, this could cause issues. Consider adding a check for Shakapacker availability.
| def self.supports_async_loading? | |
| def self.shakapacker_available? | |
| Gem.loaded_specs.key?("shakapacker") | |
| end | |
| def self.supports_async_loading? | |
| return false unless shakapacker_available? |
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.
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.
Claude finished @justin808's task —— View job
Thread Safety Analysis of @version_checks in PackerUtils
I've reviewed the concern raised about the thread safety of @version_checks on line 37 of lib/react_on_rails/packer_utils.rb.
Analysis
The @version_checks variable is indeed an instance variable at the module level (not a class variable as mentioned in the review comment). Here's what's happening:
def self.shakapacker_version_requirement_met?(required_version)
@version_checks ||= {}
@version_checks[required_version] ||= Gem::Version.new(shakapacker_version) >= Gem::Version.new(required_version)
endIs This a Real Concern?
In practice, this is not a significant issue for React on Rails for the following reasons:
-
Rails initialization context: This method is primarily called during Rails initialization and configuration setup, which happens in a single-threaded context before the application starts handling requests.
-
Immutable results: The version check results are deterministic and immutable - the same version comparison will always yield the same result within a single Ruby process.
-
Read-heavy pattern: After initialization, these values are only read, not written. The worst-case scenario in a race condition would be duplicate computation of the same result, not data corruption.
-
No state mutation: The values stored are simple booleans that don't change once computed.
Potential Issues (Theoretical)
While unlikely to cause problems in practice, theoretically there could be:
- Memory visibility issues: Without proper synchronization, one thread might not see updates made by another thread
- Duplicate computation: Multiple threads could compute the same version check simultaneously (minor performance impact)
Recommendation
Given the usage pattern of this code (initialization-time checks), I don't believe this needs immediate fixing. However, if you want to be absolutely correct about thread safety, you have a few options:
- Option 1: Use a mutex (overly cautious for this use case):
MUTEX = Mutex.new
def self.shakapacker_version_requirement_met?(required_version)
MUTEX.synchronize do
@version_checks ||= {}
@version_checks[required_version] ||= Gem::Version.new(shakapacker_version) >= Gem::Version.new(required_version)
end
end- Option 2: Use Concurrent::Hash (adds a dependency):
require 'concurrent-ruby'
@version_checks = Concurrent::Hash.new- Option 3: Leave as-is (recommended) - The current implementation is pragmatic and sufficient for the actual usage patterns in Rails applications.
Verdict
This is a valid observation from a strict thread-safety perspective, but it's not a practical concern for this codebase. The code is safe in its actual usage context (Rails initialization). I would recommend leaving it as-is unless you're building a highly concurrent system that needs to check versions dynamically at runtime (which doesn't appear to be the case).
Todo List:
- Read packer_utils.rb to understand the context
- Analyze the thread safety concern about
@version_checks - Provide review feedback on whether this needs fixing
Outdated
Copilot
AI
Sep 22, 2025
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.
The method calls packer.config.respond_to?(:nested_entries?) but packer could return nil if Shakapacker is not available, causing a NoMethodError. Add a nil check for packer before accessing its config.
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |||||||||||||
| require "active_support/core_ext/string" | ||||||||||||||
|
|
||||||||||||||
| module ReactOnRails | ||||||||||||||
| module Utils # rubocop:disable Metrics/ModuleLength | ||||||||||||||
| module Utils | ||||||||||||||
| TRUNCATION_FILLER = "\n... TRUNCATED #{ | ||||||||||||||
| Rainbow('To see the full output, set FULL_TEXT_ERRORS=true.').red | ||||||||||||||
| } ...\n".freeze | ||||||||||||||
|
|
@@ -78,22 +78,20 @@ def self.bundle_js_file_path(bundle_name) | |||||||||||||
| # a. The webpack manifest plugin would have a race condition where the same manifest.json | ||||||||||||||
| # is edited by both the webpack-dev-server | ||||||||||||||
| # b. There is no good reason to hash the server bundle name. | ||||||||||||||
| if ReactOnRails::PackerUtils.using_packer? && bundle_name != "manifest.json" | ||||||||||||||
| if bundle_name == "manifest.json" | ||||||||||||||
| # Default to the non-hashed name in the specified output directory, which, for legacy | ||||||||||||||
| # React on Rails, this is the output directory picked up by the asset pipeline. | ||||||||||||||
| # For Shakapacker, this is the public output path defined in the (shaka/web)packer.yml file. | ||||||||||||||
|
Comment on lines
+82
to
+84
|
||||||||||||||
| # Default to the non-hashed name in the specified output directory, which, for legacy | |
| # React on Rails, this is the output directory picked up by the asset pipeline. | |
| # For Shakapacker, this is the public output path defined in the (shaka/web)packer.yml file. | |
| # Default to the non-hashed name in the output directory as defined by Shakapacker's | |
| # public output path in the (shaka/web)packer.yml file. | |
| # Legacy React on Rails behavior is no longer supported; Shakapacker is now required. |
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.
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.
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.
Can now be simplified to