-
-
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 24 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,14 @@ | |||||||||||||
|
|
||||||||||||||
| 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 | ||||||||||||||
| @packer ||= begin | ||||||||||||||
| require "shakapacker" | ||||||||||||||
| ::Shakapacker | ||||||||||||||
| end | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
| def self.dev_server_running? | ||||||||||||||
| return false unless using_packer? | ||||||||||||||
|
|
||||||||||||||
| packer.dev_server.running? | ||||||||||||||
| end | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -38,7 +19,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 23, 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 @version_checks class variable is not thread-safe. In a multi-threaded environment, concurrent access could lead to race conditions. Consider using a thread-safe alternative like Concurrent::Hash or adding proper synchronization.
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 for @version_checks
I've analyzed the thread safety concern raised about the @version_checks variable on line 37 of lib/react_on_rails/packer_utils.rb.
Todo List:
- Read the packer_utils.rb file to understand the context
- Analyze the thread safety implications
- Provide assessment and recommendation
Analysis
The @version_checks instance variable is used as a cache for version comparisons:
def self.shakapacker_version_requirement_met?(required_version)
@version_checks ||= {}
@version_checks[required_version] ||= Gem::Version.new(shakapacker_version) >= Gem::Version.new(required_version)
endAssessment
This is NOT a real concern in practice for React on Rails. Here's why:
-
Rails Context: This code runs during Rails initialization and configuration setup, which happens in a single-threaded context before any request handling begins. The version checks are performed during:
- Rails boot/initialization
- Configuration validation
- Asset precompilation
-
Immutable Data: The cached values are boolean results of version comparisons. Once a version is checked, the result never changes during the application lifecycle since gem versions don't change at runtime.
-
Read-heavy Pattern: After initialization, this becomes essentially read-only data. Even if multiple threads read simultaneously after initialization, they would all compute the same boolean result.
-
Worst Case: In the extremely unlikely event of a race condition during initialization (which shouldn't happen in Rails), the worst outcome would be redundant computation of the same boolean value - not data corruption or incorrect results.
Recommendation
No action needed. The current implementation is safe for its intended use in Rails applications. Adding thread synchronization (like Mutex or Concurrent::Hash) would:
- Add unnecessary complexity
- Introduce performance overhead
- Provide no practical benefit given the initialization-time usage pattern
The code is appropriate as-is for the React on Rails gem's architecture and usage patterns.
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