Skip to content

Commit 33a2a60

Browse files
justin808claude
andcommitted
Address PR review feedback: fix bugs and improve version checking
- Fix wrong version in pro package install command (use pro gem version instead of base gem version for react-on-rails-pro) - Fix sync_versions skipping range specs (^16.5.0) when normalized version already matches by tracking non_exact flag in parse_supported_spec - Restrict strip_range_prefix to forward-looking ranges (^, ~, >=, >), treating upper-bound constraints (<, <=) as unsupported - Reorder auto_fix_versions before wildcard checks so FIX=true resolved issues don't appear as errors - Check all four dependency sections (including optionalDependencies and peerDependencies) in doctor npm wildcard check - Remove duplicate wildcard checks from system_checker (now solely in doctor) - Skip workspace:/file:/link: specs in version sync check - Use EXACT_VERSION_REGEX whitelist for npm version detection (catches tags like "latest", "beta" that the old blacklist missed) - Use Gem::Requirement.exact? for Gemfile constraint detection (catches all non-exact operators including >, <, <=) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 68f1ce8 commit 33a2a60

4 files changed

Lines changed: 56 additions & 24 deletions

File tree

react_on_rails/lib/react_on_rails/doctor.rb

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -144,9 +144,10 @@ def check_environment
144144
def check_react_on_rails_versions
145145
# Use system_checker for comprehensive package validation instead of duplicating
146146
checker.check_react_on_rails_packages
147-
check_version_wildcards
148147
check_pro_package_consistency
148+
# Auto-fix before wildcard checks so resolved issues don't appear as errors
149149
auto_fix_versions if fix
150+
check_version_wildcards
150151
end
151152

152153
def check_packages
@@ -528,18 +529,35 @@ def check_gem_wildcard_for(gemfile_content, gem_name)
528529
react_line = gemfile_content.lines.find { |line| line.match(/^\s*gem\s+['"]#{gem_name}['"]/) }
529530
return unless react_line
530531

531-
if /['"][~^]/.match?(react_line) || />=\s*/.match?(react_line)
532+
version_match = react_line.match(/gem\s+['"]#{gem_name}['"]\s*,\s*['"]([^'"]+)['"]/)
533+
unless version_match
534+
checker.add_success("✅ Gemfile uses exact version for #{gem_name}")
535+
return
536+
end
537+
538+
exact = begin
539+
Gem::Requirement.new(version_match[1]).exact?
540+
rescue Gem::Requirement::BadRequirementError
541+
false
542+
end
543+
544+
if exact
545+
checker.add_success("✅ Gemfile uses exact version for #{gem_name}")
546+
else
547+
version = if gem_name == "react_on_rails_pro"
548+
ReactOnRails::Utils.react_on_rails_pro_version
549+
else
550+
ReactOnRails::VERSION
551+
end
532552
checker.add_error(<<~MSG.strip)
533553
🚫 Gemfile uses a non-exact version constraint for #{gem_name}.
534554
535555
React on Rails requires exact version matching between the gem and npm package.
536-
Non-exact constraints (~ ^ >=) can cause the gem and npm package to drift apart.
556+
Non-exact constraints can cause the gem and npm package to drift apart.
537557
538558
Fix: Use an exact version in your Gemfile:
539-
gem '#{gem_name}', '#{gem_name == 'react_on_rails_pro' ? ReactOnRails::Utils.react_on_rails_pro_version : ReactOnRails::VERSION}'
559+
gem '#{gem_name}', '#{version}'
540560
MSG
541-
else
542-
checker.add_success("✅ Gemfile uses exact version for #{gem_name}")
543561
end
544562
end
545563

@@ -549,7 +567,9 @@ def check_npm_wildcards
549567

550568
begin
551569
package_json = JSON.parse(File.read(package_json_path))
552-
all_deps = package_json["dependencies"]&.merge(package_json["devDependencies"] || {}) || {}
570+
all_deps = ReactOnRails::VersionSynchronizer::PACKAGE_SECTIONS.each_with_object({}) do |section, deps|
571+
deps.merge!(package_json[section] || {})
572+
end
553573

554574
check_npm_wildcard_for(all_deps, "react-on-rails")
555575
check_npm_wildcard_for(all_deps, "react-on-rails-pro") if ReactOnRails::Utils.react_on_rails_pro?
@@ -564,9 +584,16 @@ def check_npm_wildcard_for(all_deps, package_name)
564584
npm_version = all_deps[package_name]
565585
return unless npm_version
566586

567-
if /[~^><*]/.match?(npm_version) || npm_version.include?(" ")
587+
if ReactOnRails::VersionSynchronizer::EXACT_VERSION_REGEX.match?(npm_version)
588+
checker.add_success("✅ package.json uses exact version for #{package_name}")
589+
else
590+
gem_version = if package_name == "react-on-rails-pro"
591+
ReactOnRails::Utils.react_on_rails_pro_version
592+
else
593+
ReactOnRails::VERSION
594+
end
568595
install_cmd = ReactOnRails::Utils.package_manager_install_exact_command(
569-
package_name, ReactOnRails::VersionSyntaxConverter.new.rubygem_to_npm(ReactOnRails::VERSION)
596+
package_name, ReactOnRails::VersionSyntaxConverter.new.rubygem_to_npm(gem_version)
570597
)
571598
checker.add_error(<<~MSG.strip)
572599
🚫 package.json uses a non-exact version for #{package_name}: #{npm_version}
@@ -577,8 +604,6 @@ def check_npm_wildcard_for(all_deps, package_name)
577604
Fix: #{install_cmd}
578605
Or run: bundle exec rake react_on_rails:sync_versions WRITE=true
579606
MSG
580-
else
581-
checker.add_success("✅ package.json uses exact version for #{package_name}")
582607
end
583608
end
584609

react_on_rails/lib/react_on_rails/system_checker.rb

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,11 +169,12 @@ def check_shakapacker_in_gemfile
169169
end
170170

171171
# React on Rails package validation
172+
# NOTE: Wildcard/non-exact version checks (Gemfile and npm) are handled by
173+
# Doctor#check_version_wildcards to avoid duplicate error messages.
172174
def check_react_on_rails_packages
173175
check_react_on_rails_gem
174176
check_react_on_rails_npm_package
175177
check_package_version_sync
176-
check_gemfile_version_patterns
177178
end
178179

179180
def check_react_on_rails_gem
@@ -221,6 +222,9 @@ def check_package_version_sync
221222

222223
return unless npm_version && defined?(ReactOnRails::VERSION)
223224

225+
# Skip workspace/local-link specs that cannot be compared or rewritten
226+
return if npm_version.match?(/\A(?:workspace:|file:|link:)/)
227+
224228
# Normalize NPM version format to Ruby gem format for comparison
225229
# Uses existing VersionSyntaxConverter to handle dash/dot differences
226230
# (e.g., "16.2.0-beta.10" → "16.2.0.beta.10")
@@ -230,7 +234,6 @@ def check_package_version_sync
230234

231235
if normalized_npm_version == gem_version
232236
add_success("✅ React on Rails gem and #{package_name} NPM package versions match (#{gem_version})")
233-
check_version_patterns(npm_version, gem_version)
234237
else
235238
# Check for major version differences
236239
gem_major = gem_version.split(".")[0].to_i

react_on_rails/lib/react_on_rails/version_synchronizer.rb

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ def parse_package_json
5959
raise ReactOnRails::Error, "Unable to read #{package_json_path}: #{e.message}"
6060
end
6161

62-
def detect_changes(package_json_data)
62+
def detect_changes(package_json_data) # rubocop:disable Metrics/CyclomaticComplexity
6363
expected_versions = expected_package_versions
6464
changes = []
6565
unsupported_specs = []
@@ -85,7 +85,7 @@ def detect_changes(package_json_data)
8585
next
8686
end
8787
normalized_current_version = converter.rubygem_to_npm(parsed_spec[:version])
88-
next if normalized_current_version == expected_version
88+
next if normalized_current_version == expected_version && !parsed_spec[:non_exact]
8989

9090
changes << {
9191
section: section,
@@ -146,18 +146,20 @@ def exact_version?(version)
146146
version.is_a?(String) && version.match?(EXACT_VERSION_REGEX)
147147
end
148148

149-
# Strips semver range prefixes (^, ~, >=, etc.) and returns the bare version if valid.
149+
# Strips forward-looking semver range prefixes (^, ~, >=, >) and returns the bare version.
150+
# Excludes < and <= because those are upper-bound constraints with different semantics
151+
# that should not be silently pinned to an exact version.
150152
def strip_range_prefix(version_spec)
151153
return nil unless version_spec.is_a?(String)
152154

153-
stripped = version_spec.sub(/\A[~^>=<]+\s*/, "")
154-
return stripped if exact_version?(stripped)
155+
stripped = version_spec.sub(/\A(?:\^|~|>=?)\s*/, "")
156+
return stripped if stripped != version_spec && exact_version?(stripped)
155157

156158
nil
157159
end
158160

159161
def parse_supported_spec(version_spec)
160-
return { version: version_spec, prefix: nil } if exact_version?(version_spec)
162+
return { version: version_spec, prefix: nil, non_exact: false } if exact_version?(version_spec)
161163

162164
# Handle npm alias syntax: npm:@scope/pkg@version
163165
if version_spec.is_a?(String) && version_spec.start_with?(NPM_ALIAS_PREFIX)
@@ -166,7 +168,7 @@ def parse_supported_spec(version_spec)
166168

167169
# Handle range prefixes: ^16.5.0, ~16.5.0, >=16.5.0, etc.
168170
stripped = strip_range_prefix(version_spec)
169-
return { version: stripped, prefix: nil } if stripped
171+
return { version: stripped, prefix: nil, non_exact: true } if stripped
170172

171173
nil
172174
end
@@ -177,11 +179,11 @@ def parse_npm_alias_spec(version_spec)
177179

178180
alias_version = version_spec[(at_index + 1)..]
179181
prefix = version_spec[0..at_index]
180-
return { version: alias_version, prefix: prefix } if exact_version?(alias_version)
182+
return { version: alias_version, prefix: prefix, non_exact: false } if exact_version?(alias_version)
181183

182184
# Try stripping range prefix from alias version (e.g., npm:@scope/pkg@^16.5.0)
183185
stripped = strip_range_prefix(alias_version)
184-
return { version: stripped, prefix: prefix } if stripped
186+
return { version: stripped, prefix: prefix, non_exact: true } if stripped
185187

186188
nil
187189
end

react_on_rails/spec/lib/react_on_rails/system_checker_spec.rb

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -440,12 +440,14 @@
440440
allow(File).to receive(:read).with("package.json").and_return(package_json_content)
441441
end
442442

443-
it "adds a success message and version pattern error" do
443+
it "adds a success message (wildcard detection is handled by Doctor)" do
444444
checker.send(:check_package_version_sync)
445445
expect(checker.messages.any? do |msg|
446446
msg[:type] == :success && msg[:content].include?("versions match")
447447
end).to be true
448-
expect(checker.errors?).to be true
448+
# Version pattern errors are now detected by Doctor#check_version_wildcards
449+
# to avoid duplicate error messages
450+
expect(checker.errors?).to be false
449451
end
450452
end
451453

0 commit comments

Comments
 (0)