From 1a75fcbad1172e856521f900276e81cdf12855cd Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Tue, 4 Nov 2025 21:24:55 -1000 Subject: [PATCH 1/2] Improve validation skip documentation and test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../react_on_rails/install_generator.rb | 16 ++++++++ lib/react_on_rails/engine.rb | 14 +++++++ spec/react_on_rails/engine_spec.rb | 37 +++++++++++++++++++ 3 files changed, 67 insertions(+) diff --git a/lib/generators/react_on_rails/install_generator.rb b/lib/generators/react_on_rails/install_generator.rb index eee693f4b9..8e4dde3d8b 100644 --- a/lib/generators/react_on_rails/install_generator.rb +++ b/lib/generators/react_on_rails/install_generator.rb @@ -36,9 +36,23 @@ class InstallGenerator < Rails::Generators::Base # Removed: --skip-shakapacker-install (Shakapacker is now a required dependency) + # Main generator entry point + # + # Sets up React on Rails in a Rails application by: + # 1. Validating prerequisites + # 2. Installing required packages + # 3. Generating configuration files + # 4. Setting up example components + # + # @note Validation Skipping: Sets ENV["REACT_ON_RAILS_SKIP_VALIDATION"] to prevent + # version validation from running during generator execution. The npm package + # isn't installed until midway through the generator, so validation would fail + # if run during Rails initialization. The ensure block guarantees cleanup even + # if the generator fails. def run_generators # Set environment variable to skip validation during generator run # This is inherited by all invoked generators and persists through Rails initialization + # See lib/react_on_rails/engine.rb for the validation skip logic ENV["REACT_ON_RAILS_SKIP_VALIDATION"] = "true" if installation_prerequisites_met? || options.ignore_warnings? @@ -59,6 +73,8 @@ def run_generators GeneratorMessages.add_error(error) end ensure + # Always clean up ENV variable to avoid affecting subsequent processes + # This is safe even if concurrent generators run in separate processes ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION") print_generator_messages end diff --git a/lib/react_on_rails/engine.rb b/lib/react_on_rails/engine.rb index eee194ae60..fe5ee04769 100644 --- a/lib/react_on_rails/engine.rb +++ b/lib/react_on_rails/engine.rb @@ -18,9 +18,22 @@ class Engine < ::Rails::Engine end # Determine if version validation should be skipped + # + # This method checks multiple conditions to determine if package version validation + # should be skipped. Validation is skipped during setup scenarios where the npm + # package isn't installed yet (e.g., during generator execution). + # # @return [Boolean] true if validation should be skipped + # + # @note Thread Safety: ENV variables are process-global. In practice, Rails generators + # run in a single process, so concurrent execution is not a concern. If running + # generators concurrently (e.g., in parallel tests), ensure tests run in separate + # processes to avoid ENV variable conflicts. def self.skip_version_validation? # Skip if explicitly disabled via environment variable (set by generators) + # Using ENV variable instead of ARGV because Rails can modify/clear ARGV during + # initialization, making ARGV unreliable for detecting generator context. The ENV + # variable persists through the entire Rails initialization process. if ENV["REACT_ON_RAILS_SKIP_VALIDATION"] == "true" Rails.logger.debug "[React on Rails] Skipping validation - disabled via environment variable" return true @@ -33,6 +46,7 @@ def self.skip_version_validation? end # Skip during generator runtime since packages are installed during execution + # This is a fallback check in case ENV wasn't set, though ENV is the primary mechanism if running_generator? Rails.logger.debug "[React on Rails] Skipping validation during generator runtime" return true diff --git a/spec/react_on_rails/engine_spec.rb b/spec/react_on_rails/engine_spec.rb index cce8f24b88..fdc138493d 100644 --- a/spec/react_on_rails/engine_spec.rb +++ b/spec/react_on_rails/engine_spec.rb @@ -31,6 +31,43 @@ module ReactOnRails expect(Rails.logger).to have_received(:debug) .with("[React on Rails] Skipping validation - disabled via environment variable") end + + context "with package.json present and ARGV set to generator" do + before do + allow(File).to receive(:exist?).with(package_json_path).and_return(true) + stub_const("ARGV", ["generate", "react_on_rails:install"]) + end + + it "returns true (ENV takes precedence over other conditions)" do + expect(described_class.skip_version_validation?).to be true + end + + it "logs ENV variable message, not ARGV message" do + described_class.skip_version_validation? + expect(Rails.logger).to have_received(:debug) + .with("[React on Rails] Skipping validation - disabled via environment variable") + expect(Rails.logger).not_to have_received(:debug) + .with("[React on Rails] Skipping validation during generator runtime") + end + end + + context "with package.json missing" do + before do + allow(File).to receive(:exist?).with(package_json_path).and_return(false) + end + + it "returns true (ENV takes precedence over package.json check)" do + expect(described_class.skip_version_validation?).to be true + end + + it "logs ENV variable message, not package.json message" do + described_class.skip_version_validation? + expect(Rails.logger).to have_received(:debug) + .with("[React on Rails] Skipping validation - disabled via environment variable") + expect(Rails.logger).not_to have_received(:debug) + .with("[React on Rails] Skipping validation - package.json not found") + end + end end context "when package.json doesn't exist" do From bdb7307e0dd616965e6866d2ff104bd4ba3b89cf Mon Sep 17 00:00:00 2001 From: Justin Gordon Date: Tue, 4 Nov 2025 21:43:57 -1000 Subject: [PATCH 2/2] Refine documentation and test structure per code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses additional code review suggestions: 1. Edge Case Documentation: - Added @note about manual ENV variable setting - Documented use case for debugging/setup scenarios - Warned against bypassing validation in production 2. Improved Test Structure: - Refactored precedence tests to use nested contexts - Changed structure: "with other skip conditions also present" -> "when package.json exists and ARGV indicates generator" -> "when package.json is missing" - More descriptive test names focusing on behavior - Better organization showing ENV precedence clearly 3. Simplified Comments: - Reduced redundancy in install_generator.rb ensure block - Changed from 2 lines to 1 concise line - Removed redundant statement about concurrent generators Changes maintain all functionality while improving readability and addressing edge cases in documentation. All specs pass (21 examples, 0 failures) RuboCop clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../react_on_rails/install_generator.rb | 3 +- lib/react_on_rails/engine.rb | 6 ++ spec/react_on_rails/engine_spec.rb | 58 ++++++++++--------- 3 files changed, 37 insertions(+), 30 deletions(-) diff --git a/lib/generators/react_on_rails/install_generator.rb b/lib/generators/react_on_rails/install_generator.rb index 8e4dde3d8b..3a7e351202 100644 --- a/lib/generators/react_on_rails/install_generator.rb +++ b/lib/generators/react_on_rails/install_generator.rb @@ -73,8 +73,7 @@ def run_generators GeneratorMessages.add_error(error) end ensure - # Always clean up ENV variable to avoid affecting subsequent processes - # This is safe even if concurrent generators run in separate processes + # Always clean up ENV variable, even if generator fails ENV.delete("REACT_ON_RAILS_SKIP_VALIDATION") print_generator_messages end diff --git a/lib/react_on_rails/engine.rb b/lib/react_on_rails/engine.rb index fe5ee04769..a7abdd4a77 100644 --- a/lib/react_on_rails/engine.rb +++ b/lib/react_on_rails/engine.rb @@ -29,6 +29,12 @@ class Engine < ::Rails::Engine # run in a single process, so concurrent execution is not a concern. If running # generators concurrently (e.g., in parallel tests), ensure tests run in separate # processes to avoid ENV variable conflicts. + # + # @note Manual ENV Setting: While this ENV variable is designed to be set by generators, + # users can manually set it (e.g., `REACT_ON_RAILS_SKIP_VALIDATION=true rails server`) + # to bypass validation. This should only be done temporarily during debugging or + # setup scenarios. The validation helps catch version mismatches early, so bypassing + # it in production is not recommended. def self.skip_version_validation? # Skip if explicitly disabled via environment variable (set by generators) # Using ENV variable instead of ARGV because Rails can modify/clear ARGV during diff --git a/spec/react_on_rails/engine_spec.rb b/spec/react_on_rails/engine_spec.rb index fdc138493d..069bf53c5e 100644 --- a/spec/react_on_rails/engine_spec.rb +++ b/spec/react_on_rails/engine_spec.rb @@ -32,40 +32,42 @@ module ReactOnRails .with("[React on Rails] Skipping validation - disabled via environment variable") end - context "with package.json present and ARGV set to generator" do - before do - allow(File).to receive(:exist?).with(package_json_path).and_return(true) - stub_const("ARGV", ["generate", "react_on_rails:install"]) - end + context "with other skip conditions also present" do + context "when package.json exists and ARGV indicates generator" do + before do + allow(File).to receive(:exist?).with(package_json_path).and_return(true) + stub_const("ARGV", ["generate", "react_on_rails:install"]) + end - it "returns true (ENV takes precedence over other conditions)" do - expect(described_class.skip_version_validation?).to be true - end + it "prioritizes ENV over ARGV check" do + expect(described_class.skip_version_validation?).to be true + end - it "logs ENV variable message, not ARGV message" do - described_class.skip_version_validation? - expect(Rails.logger).to have_received(:debug) - .with("[React on Rails] Skipping validation - disabled via environment variable") - expect(Rails.logger).not_to have_received(:debug) - .with("[React on Rails] Skipping validation during generator runtime") + it "short-circuits before checking ARGV" do + described_class.skip_version_validation? + expect(Rails.logger).to have_received(:debug) + .with("[React on Rails] Skipping validation - disabled via environment variable") + expect(Rails.logger).not_to have_received(:debug) + .with("[React on Rails] Skipping validation during generator runtime") + end end - end - context "with package.json missing" do - before do - allow(File).to receive(:exist?).with(package_json_path).and_return(false) - end + context "when package.json is missing" do + before do + allow(File).to receive(:exist?).with(package_json_path).and_return(false) + end - it "returns true (ENV takes precedence over package.json check)" do - expect(described_class.skip_version_validation?).to be true - end + it "prioritizes ENV over package.json check" do + expect(described_class.skip_version_validation?).to be true + end - it "logs ENV variable message, not package.json message" do - described_class.skip_version_validation? - expect(Rails.logger).to have_received(:debug) - .with("[React on Rails] Skipping validation - disabled via environment variable") - expect(Rails.logger).not_to have_received(:debug) - .with("[React on Rails] Skipping validation - package.json not found") + it "short-circuits before checking package.json" do + described_class.skip_version_validation? + expect(Rails.logger).to have_received(:debug) + .with("[React on Rails] Skipping validation - disabled via environment variable") + expect(Rails.logger).not_to have_received(:debug) + .with("[React on Rails] Skipping validation - package.json not found") + end end end end