Skip to content

Commit 1a75fcb

Browse files
justin808claude
andcommitted
Improve validation skip documentation and test coverage
Addresses feedback from PR #1923: 1. Enhanced documentation: - Added comprehensive method-level docs explaining ENV variable approach - Documented why ENV is preferred over ARGV (Rails modifies ARGV during init) - Added thread safety notes explaining process-global ENV scope - Cross-referenced engine.rb and install_generator.rb for clarity 2. Improved test coverage: - Added tests verifying ENV variable takes precedence over other checks - Tests confirm ENV check short-circuits before package.json/ARGV checks - Added 4 new test cases covering precedence scenarios 3. Thread safety documentation: - Documented that generators run in single process (not a concern) - Added note about parallel test scenarios (separate processes) - Clarified ENV cleanup in ensure block Changes: - lib/react_on_rails/engine.rb: Enhanced skip_validation? docs - lib/generators/react_on_rails/install_generator.rb: Added run_generators docs - spec/react_on_rails/engine_spec.rb: Added 4 precedence tests All specs pass (21 examples, 0 failures) RuboCop clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent ae5425b commit 1a75fcb

File tree

3 files changed

+67
-0
lines changed

3 files changed

+67
-0
lines changed

lib/generators/react_on_rails/install_generator.rb

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,23 @@ class InstallGenerator < Rails::Generators::Base
3636

3737
# Removed: --skip-shakapacker-install (Shakapacker is now a required dependency)
3838

39+
# Main generator entry point
40+
#
41+
# Sets up React on Rails in a Rails application by:
42+
# 1. Validating prerequisites
43+
# 2. Installing required packages
44+
# 3. Generating configuration files
45+
# 4. Setting up example components
46+
#
47+
# @note Validation Skipping: Sets ENV["REACT_ON_RAILS_SKIP_VALIDATION"] to prevent
48+
# version validation from running during generator execution. The npm package
49+
# isn't installed until midway through the generator, so validation would fail
50+
# if run during Rails initialization. The ensure block guarantees cleanup even
51+
# if the generator fails.
3952
def run_generators
4053
# Set environment variable to skip validation during generator run
4154
# This is inherited by all invoked generators and persists through Rails initialization
55+
# See lib/react_on_rails/engine.rb for the validation skip logic
4256
ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true"
4357

4458
if installation_prerequisites_met? || options.ignore_warnings?
@@ -59,6 +73,8 @@ def run_generators
5973
GeneratorMessages.add_error(error)
6074
end
6175
ensure
76+
# Always clean up ENV variable to avoid affecting subsequent processes
77+
# This is safe even if concurrent generators run in separate processes
6278
ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION")
6379
print_generator_messages
6480
end

lib/react_on_rails/engine.rb

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,22 @@ class Engine < ::Rails::Engine
1818
end
1919

2020
# Determine if version validation should be skipped
21+
#
22+
# This method checks multiple conditions to determine if package version validation
23+
# should be skipped. Validation is skipped during setup scenarios where the npm
24+
# package isn't installed yet (e.g., during generator execution).
25+
#
2126
# @return [Boolean] true if validation should be skipped
27+
#
28+
# @note Thread Safety: ENV variables are process-global. In practice, Rails generators
29+
# run in a single process, so concurrent execution is not a concern. If running
30+
# generators concurrently (e.g., in parallel tests), ensure tests run in separate
31+
# processes to avoid ENV variable conflicts.
2232
def self.skip_version_validation?
2333
# Skip if explicitly disabled via environment variable (set by generators)
34+
# Using ENV variable instead of ARGV because Rails can modify/clear ARGV during
35+
# initialization, making ARGV unreliable for detecting generator context. The ENV
36+
# variable persists through the entire Rails initialization process.
2437
if ENV["REACT_ON_RAILS_SKIP_VALIDATION"] == "true"
2538
Rails.logger.debug "[React on Rails] Skipping validation - disabled via environment variable"
2639
return true
@@ -33,6 +46,7 @@ def self.skip_version_validation?
3346
end
3447

3548
# Skip during generator runtime since packages are installed during execution
49+
# This is a fallback check in case ENV wasn't set, though ENV is the primary mechanism
3650
if running_generator?
3751
Rails.logger.debug "[React on Rails] Skipping validation during generator runtime"
3852
return true

spec/react_on_rails/engine_spec.rb

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,43 @@ module ReactOnRails
3131
expect(Rails.logger).to have_received(:debug)
3232
.with("[React on Rails] Skipping validation - disabled via environment variable")
3333
end
34+
35+
context "with package.json present and ARGV set to generator" do
36+
before do
37+
allow(File).to receive(:exist?).with(package_json_path).and_return(true)
38+
stub_const("ARGV", ["generate", "react_on_rails:install"])
39+
end
40+
41+
it "returns true (ENV takes precedence over other conditions)" do
42+
expect(described_class.skip_version_validation?).to be true
43+
end
44+
45+
it "logs ENV variable message, not ARGV message" do
46+
described_class.skip_version_validation?
47+
expect(Rails.logger).to have_received(:debug)
48+
.with("[React on Rails] Skipping validation - disabled via environment variable")
49+
expect(Rails.logger).not_to have_received(:debug)
50+
.with("[React on Rails] Skipping validation during generator runtime")
51+
end
52+
end
53+
54+
context "with package.json missing" do
55+
before do
56+
allow(File).to receive(:exist?).with(package_json_path).and_return(false)
57+
end
58+
59+
it "returns true (ENV takes precedence over package.json check)" do
60+
expect(described_class.skip_version_validation?).to be true
61+
end
62+
63+
it "logs ENV variable message, not package.json message" do
64+
described_class.skip_version_validation?
65+
expect(Rails.logger).to have_received(:debug)
66+
.with("[React on Rails] Skipping validation - disabled via environment variable")
67+
expect(Rails.logger).not_to have_received(:debug)
68+
.with("[React on Rails] Skipping validation - package.json not found")
69+
end
70+
end
3471
end
3572

3673
context "when package.json doesn't exist" do

0 commit comments

Comments
 (0)