diff --git a/lib/react_on_rails/configuration.rb b/lib/react_on_rails/configuration.rb index 20be673d42..ffe707ad6c 100644 --- a/lib/react_on_rails/configuration.rb +++ b/lib/react_on_rails/configuration.rb @@ -238,7 +238,14 @@ def adjust_precompile_task raise(ReactOnRails::Error, compile_command_conflict_message) if ReactOnRails::PackerUtils.precompile? precompile_tasks = lambda { - Rake::Task["react_on_rails:generate_packs"].invoke + # Skip generate_packs if shakapacker has a precompile hook configured + if ReactOnRails::PackerUtils.shakapacker_precompile_hook_configured? + hook_value = ReactOnRails::PackerUtils.shakapacker_precompile_hook_value + puts "Skipping react_on_rails:generate_packs (configured in shakapacker precompile hook: #{hook_value})" + else + Rake::Task["react_on_rails:generate_packs"].invoke + end + Rake::Task["react_on_rails:assets:webpack"].invoke # VERSIONS is per the shakacode/shakapacker clean method definition. diff --git a/lib/react_on_rails/dev/pack_generator.rb b/lib/react_on_rails/dev/pack_generator.rb index de3a4ad626..a339d5860f 100644 --- a/lib/react_on_rails/dev/pack_generator.rb +++ b/lib/react_on_rails/dev/pack_generator.rb @@ -5,9 +5,36 @@ module ReactOnRails module Dev + # PackGenerator triggers the generation of React on Rails packs + # + # Design decisions: + # 1. Why trigger via Rake task instead of direct Ruby code? + # - The actual pack generation logic lives in lib/react_on_rails/packs_generator.rb + # - The rake task (lib/tasks/generate_packs.rake) provides a stable, documented interface + # - This allows the implementation to evolve without breaking bin/dev + # - Users can also run the task directly: `rake react_on_rails:generate_packs` + # + # 2. Why two execution strategies (direct vs bundle exec)? + # - Direct Rake execution: Faster when already in Bundler/Rails context (bin/dev) + # - Bundle exec fallback: Required when called outside Rails context + # - This optimization avoids subprocess overhead in the common case + # + # 3. Why is the class named "PackGenerator" when it delegates? + # - It's a semantic wrapper around pack generation for the dev workflow + # - Provides a clean API for bin/dev without exposing Rake internals + # - Handles hook detection, error handling, and output formatting class PackGenerator class << self def generate(verbose: false) + # Skip if shakapacker has a precompile hook configured + if ReactOnRails::PackerUtils.shakapacker_precompile_hook_configured? + if verbose + hook_value = ReactOnRails::PackerUtils.shakapacker_precompile_hook_value + puts "⏭️ Skipping pack generation (handled by shakapacker precompile hook: #{hook_value})" + end + return + end + if verbose puts "📦 Generating React on Rails packs..." success = run_pack_generation diff --git a/lib/react_on_rails/packer_utils.rb b/lib/react_on_rails/packer_utils.rb index d6e6511000..e4a05ef761 100644 --- a/lib/react_on_rails/packer_utils.rb +++ b/lib/react_on_rails/packer_utils.rb @@ -166,5 +166,78 @@ def self.raise_shakapacker_version_incompatible_for_basic_pack_generation raise ReactOnRails::Error, msg end + + # Check if shakapacker.yml has a precompile hook configured + # This prevents react_on_rails from running generate_packs twice + # + # Returns false if detection fails for any reason (missing shakapacker, malformed config, etc.) + # to ensure generate_packs runs rather than being incorrectly skipped + # + # Note: Currently checks a single hook value. Future enhancement will support hook lists + # to allow prepending/appending multiple commands. See related Shakapacker issue for details. + def self.shakapacker_precompile_hook_configured? + return false unless defined?(::Shakapacker) + + hook_value = extract_precompile_hook + return false if hook_value.nil? + + hook_contains_generate_packs?(hook_value) + rescue StandardError => e + # Swallow errors during hook detection to fail safe - if we can't detect the hook, + # we should run generate_packs rather than skip it incorrectly. + # Possible errors: NoMethodError (config method missing), TypeError (unexpected data structure), + # or errors from shakapacker's internal implementation changes + warn "Warning: Unable to detect shakapacker precompile hook: #{e.message}" if ENV["DEBUG"] + false + end + + def self.extract_precompile_hook + # Access config data using private :data method since there's no public API + # to access the raw configuration hash needed for hook detection + config_data = ::Shakapacker.config.send(:data) + + # Try symbol keys first (Shakapacker's internal format), then fall back to string keys + # Note: Currently only one hook value is supported, but this will change to support lists + config_data&.dig(:hooks, :precompile) || config_data&.dig("hooks", "precompile") + end + + def self.hook_contains_generate_packs?(hook_value) + # The hook value can be either: + # 1. A direct command containing the rake task + # 2. A path to a script file that needs to be read + return false if hook_value.blank? + + # Check if it's a direct command first + return true if hook_value.to_s.match?(/\breact_on_rails:generate_packs\b/) + + # Check if it's a script file path + script_path = resolve_hook_script_path(hook_value) + return false unless script_path && File.exist?(script_path) + + # Read and check script contents + script_contents = File.read(script_path) + script_contents.match?(/\breact_on_rails:generate_packs\b/) + rescue StandardError + # If we can't read the script, assume it doesn't contain generate_packs + false + end + + def self.resolve_hook_script_path(hook_value) + # Hook value might be a script path relative to Rails root + return nil unless defined?(Rails) && Rails.respond_to?(:root) + + potential_path = Rails.root.join(hook_value.to_s.strip) + potential_path if potential_path.file? + end + + # Returns the configured precompile hook value for logging/debugging + # Returns nil if no hook is configured + def self.shakapacker_precompile_hook_value + return nil unless defined?(::Shakapacker) + + extract_precompile_hook + rescue StandardError + nil + end end end diff --git a/spec/react_on_rails/dev/pack_generator_spec.rb b/spec/react_on_rails/dev/pack_generator_spec.rb index 9e55cb3174..578df74991 100644 --- a/spec/react_on_rails/dev/pack_generator_spec.rb +++ b/spec/react_on_rails/dev/pack_generator_spec.rb @@ -3,168 +3,206 @@ require_relative "../spec_helper" require "react_on_rails/dev/pack_generator" +# Tests for PackGenerator.generate which triggers pack generation for react_on_rails +# +# This class is responsible for: +# 1. Detecting if shakapacker has a precompile hook configured and skipping if so +# 2. Choosing the optimal execution strategy (direct Rake task vs bundle exec) +# 3. Handling errors appropriately and providing user-friendly output +# +# Test coverage includes: +# - Shakapacker hook detection (skip generation when hook is configured) +# - Direct Rake execution in Bundler context (faster, no subprocess overhead) +# - Fallback to bundle exec when Rails/Bundler not available +# - Error handling with proper stderr output +# - Silent mode output suppression RSpec.describe ReactOnRails::Dev::PackGenerator do describe ".generate" do - context "when in Bundler context with Rails available" do - let(:mock_task) { instance_double(Rake::Task) } - let(:mock_rails_app) do - # rubocop:disable RSpec/VerifiedDoubles - double("Rails.application").tap do |app| - allow(app).to receive(:load_tasks) - allow(app).to receive(:respond_to?).with(:load_tasks).and_return(true) - end - # rubocop:enable RSpec/VerifiedDoubles - end + before do + # Mock the precompile hook check to return false by default + allow(ReactOnRails::PackerUtils).to receive(:shakapacker_precompile_hook_configured?).and_return(false) + end + context "when shakapacker precompile hook is configured" do before do - # Setup Bundler context - stub_const("Bundler", Module.new) - allow(ENV).to receive(:[]).and_call_original - allow(ENV).to receive(:[]).with("BUNDLE_GEMFILE").and_return("/path/to/Gemfile") - - # Setup Rails availability - app = mock_rails_app - rails_module = Module.new do - define_singleton_method(:application) { app } - define_singleton_method(:respond_to?) { |method, *| method == :application } - end - stub_const("Rails", rails_module) - - # Mock Rake::Task at the boundary - allow(Rake::Task).to receive(:task_defined?).with("react_on_rails:generate_packs").and_return(false) - allow(Rake::Task).to receive(:[]).with("react_on_rails:generate_packs").and_return(mock_task) - allow(mock_task).to receive(:reenable) - allow(mock_task).to receive(:invoke) + allow(ReactOnRails::PackerUtils).to receive_messages(shakapacker_precompile_hook_configured?: true, + shakapacker_precompile_hook_value: "bin/precompile_hook") end - it "runs pack generation successfully in verbose mode using direct rake execution" do + it "skips pack generation in verbose mode and shows hook value" do expect { described_class.generate(verbose: true) } - .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process - - expect(mock_task).to have_received(:invoke) - expect(mock_rails_app).to have_received(:load_tasks) + .to output(%r{⏭️ Skipping pack generation \(handled by shakapacker precompile hook: bin/precompile_hook\)}) + .to_stdout_from_any_process end - it "runs pack generation successfully in quiet mode using direct rake execution" do + it "skips pack generation in quiet mode" do expect { described_class.generate(verbose: false) } - .to output(/📦 Generating packs\.\.\. ✅/).to_stdout_from_any_process - - expect(mock_task).to have_received(:invoke) + .not_to output.to_stdout_from_any_process end + end - it "exits with error when pack generation fails" do - allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Task failed")) + context "when shakapacker precompile hook is not configured" do + context "when in Bundler context with Rails available" do + let(:mock_task) { instance_double(Rake::Task) } + let(:mock_rails_app) do + # rubocop:disable RSpec/VerifiedDoubles + double("Rails.application").tap do |app| + allow(app).to receive(:load_tasks) + allow(app).to receive(:respond_to?).with(:load_tasks).and_return(true) + end + # rubocop:enable RSpec/VerifiedDoubles + end - # Mock STDERR.puts to capture output - error_output = [] - # rubocop:disable Style/GlobalStdStream - allow(STDERR).to receive(:puts) { |msg| error_output << msg } - # rubocop:enable Style/GlobalStdStream + before do + # Setup Bundler context + stub_const("Bundler", Module.new) + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:[]).with("BUNDLE_GEMFILE").and_return("/path/to/Gemfile") + + # Setup Rails availability + app = mock_rails_app + rails_module = Module.new do + define_singleton_method(:application) { app } + define_singleton_method(:respond_to?) { |method, *| method == :application } + end + stub_const("Rails", rails_module) + + # Mock Rake::Task at the boundary + allow(Rake::Task).to receive(:task_defined?).with("react_on_rails:generate_packs").and_return(false) + allow(Rake::Task).to receive(:[]).with("react_on_rails:generate_packs").and_return(mock_task) + allow(mock_task).to receive(:reenable) + allow(mock_task).to receive(:invoke) + end - expect { described_class.generate(verbose: false) }.to raise_error(SystemExit) - expect(error_output.join("\n")).to match(/Error generating packs: Task failed/) - end + it "runs pack generation successfully in verbose mode using direct rake execution" do + expect { described_class.generate(verbose: true) } + .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process - it "outputs errors to stderr even in silent mode" do - allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Silent mode error")) + expect(mock_task).to have_received(:invoke) + expect(mock_rails_app).to have_received(:load_tasks) + end - # Mock STDERR.puts to capture output - error_output = [] - # rubocop:disable Style/GlobalStdStream - allow(STDERR).to receive(:puts) { |msg| error_output << msg } - # rubocop:enable Style/GlobalStdStream + it "runs pack generation successfully in quiet mode using direct rake execution" do + expect { described_class.generate(verbose: false) } + .to output(/📦 Generating packs\.\.\. ✅/).to_stdout_from_any_process - expect { described_class.generate(verbose: false) }.to raise_error(SystemExit) - expect(error_output.join("\n")).to match(/Error generating packs: Silent mode error/) - end + expect(mock_task).to have_received(:invoke) + end - it "includes backtrace in error output when DEBUG env is set" do - allow(ENV).to receive(:[]).with("DEBUG").and_return("true") - allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Debug error")) + it "exits with error when pack generation fails" do + allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Task failed")) - # Mock STDERR.puts to capture output - error_output = [] - # rubocop:disable Style/GlobalStdStream - allow(STDERR).to receive(:puts) { |msg| error_output << msg } - # rubocop:enable Style/GlobalStdStream + # Mock STDERR.puts to capture output + error_output = [] + # rubocop:disable Style/GlobalStdStream + allow(STDERR).to receive(:puts) { |msg| error_output << msg } + # rubocop:enable Style/GlobalStdStream - expect { described_class.generate(verbose: false) }.to raise_error(SystemExit) - expect(error_output.join("\n")).to match(/Error generating packs: Debug error.*pack_generator_spec\.rb/m) - end + expect { described_class.generate(verbose: false) }.to raise_error(SystemExit) + expect(error_output.join("\n")).to match(/Error generating packs: Task failed/) + end - it "suppresses stdout in silent mode" do - # Mock task to produce output - allow(mock_task).to receive(:invoke) do - puts "This should be suppressed" + it "outputs errors to stderr even in silent mode" do + allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Silent mode error")) + + # Mock STDERR.puts to capture output + error_output = [] + # rubocop:disable Style/GlobalStdStream + allow(STDERR).to receive(:puts) { |msg| error_output << msg } + # rubocop:enable Style/GlobalStdStream + + expect { described_class.generate(verbose: false) }.to raise_error(SystemExit) + expect(error_output.join("\n")).to match(/Error generating packs: Silent mode error/) end - expect { described_class.generate(verbose: false) } - .not_to output(/This should be suppressed/).to_stdout_from_any_process - end - end + it "includes backtrace in error output when DEBUG env is set" do + allow(ENV).to receive(:[]).with("DEBUG").and_return("true") + allow(mock_task).to receive(:invoke).and_raise(StandardError.new("Debug error")) - context "when not in Bundler context" do - before do - # Ensure we're not in Bundler context - hide_const("Bundler") if defined?(Bundler) - end + # Mock STDERR.puts to capture output + error_output = [] + # rubocop:disable Style/GlobalStdStream + allow(STDERR).to receive(:puts) { |msg| error_output << msg } + # rubocop:enable Style/GlobalStdStream - it "runs pack generation successfully in verbose mode using bundle exec" do - allow(described_class).to receive(:system) - .with("bundle", "exec", "rake", "react_on_rails:generate_packs") - .and_return(true) + expect { described_class.generate(verbose: false) }.to raise_error(SystemExit) + expect(error_output.join("\n")).to match(/Error generating packs: Debug error.*pack_generator_spec\.rb/m) + end - expect { described_class.generate(verbose: true) } - .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process + it "suppresses stdout in silent mode" do + # Mock task to produce output + allow(mock_task).to receive(:invoke) do + puts "This should be suppressed" + end - expect(described_class).to have_received(:system) - .with("bundle", "exec", "rake", "react_on_rails:generate_packs") + expect { described_class.generate(verbose: false) } + .not_to output(/This should be suppressed/).to_stdout_from_any_process + end end - it "runs pack generation successfully in quiet mode using bundle exec" do - allow(described_class).to receive(:system) - .with("bundle", "exec", "rake", "react_on_rails:generate_packs", - out: File::NULL, err: File::NULL) - .and_return(true) + context "when not in Bundler context" do + before do + # Ensure we're not in Bundler context + hide_const("Bundler") if defined?(Bundler) + end - expect { described_class.generate(verbose: false) } - .to output(/📦 Generating packs\.\.\. ✅/).to_stdout_from_any_process + it "runs pack generation successfully in verbose mode using bundle exec" do + allow(described_class).to receive(:system) + .with("bundle", "exec", "rake", "react_on_rails:generate_packs") + .and_return(true) - expect(described_class).to have_received(:system) - .with("bundle", "exec", "rake", "react_on_rails:generate_packs", - out: File::NULL, err: File::NULL) - end + expect { described_class.generate(verbose: true) } + .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process - it "exits with error when pack generation fails" do - allow(described_class).to receive(:system) - .with("bundle", "exec", "rake", "react_on_rails:generate_packs", - out: File::NULL, err: File::NULL) - .and_return(false) + expect(described_class).to have_received(:system) + .with("bundle", "exec", "rake", "react_on_rails:generate_packs") + end - expect { described_class.generate(verbose: false) }.to raise_error(SystemExit) - end - end + it "runs pack generation successfully in quiet mode using bundle exec" do + allow(described_class).to receive(:system) + .with("bundle", "exec", "rake", "react_on_rails:generate_packs", + out: File::NULL, err: File::NULL) + .and_return(true) - context "when Rails is not available" do - before do - stub_const("Bundler", Module.new) - allow(ENV).to receive(:[]).and_call_original - allow(ENV).to receive(:[]).with("BUNDLE_GEMFILE").and_return("/path/to/Gemfile") + expect { described_class.generate(verbose: false) } + .to output(/📦 Generating packs\.\.\. ✅/).to_stdout_from_any_process + + expect(described_class).to have_received(:system) + .with("bundle", "exec", "rake", "react_on_rails:generate_packs", + out: File::NULL, err: File::NULL) + end - # Rails not available - hide_const("Rails") if defined?(Rails) + it "exits with error when pack generation fails" do + allow(described_class).to receive(:system) + .with("bundle", "exec", "rake", "react_on_rails:generate_packs", + out: File::NULL, err: File::NULL) + .and_return(false) + + expect { described_class.generate(verbose: false) }.to raise_error(SystemExit) + end end - it "falls back to bundle exec when Rails is not defined" do - allow(described_class).to receive(:system) - .with("bundle", "exec", "rake", "react_on_rails:generate_packs") - .and_return(true) + context "when Rails is not available" do + before do + stub_const("Bundler", Module.new) + allow(ENV).to receive(:[]).and_call_original + allow(ENV).to receive(:[]).with("BUNDLE_GEMFILE").and_return("/path/to/Gemfile") - expect { described_class.generate(verbose: true) } - .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process + # Rails not available + hide_const("Rails") if defined?(Rails) + end + + it "falls back to bundle exec when Rails is not defined" do + allow(described_class).to receive(:system) + .with("bundle", "exec", "rake", "react_on_rails:generate_packs") + .and_return(true) - expect(described_class).to have_received(:system) - .with("bundle", "exec", "rake", "react_on_rails:generate_packs") + expect { described_class.generate(verbose: true) } + .to output(/📦 Generating React on Rails packs.../).to_stdout_from_any_process + + expect(described_class).to have_received(:system) + .with("bundle", "exec", "rake", "react_on_rails:generate_packs") + end end end end diff --git a/spec/react_on_rails/packer_utils_spec.rb b/spec/react_on_rails/packer_utils_spec.rb index 6636eaa65f..3625082124 100644 --- a/spec/react_on_rails/packer_utils_spec.rb +++ b/spec/react_on_rails/packer_utils_spec.rb @@ -2,6 +2,7 @@ require_relative "spec_helper" +# rubocop:disable Metrics/ModuleLength module ReactOnRails describe PackerUtils do describe ".shakapacker_version_requirement_met?" do @@ -139,6 +140,145 @@ module ReactOnRails expect(described_class.supports_autobundling?).to be(false) end end + + describe ".shakapacker_precompile_hook_configured?" do + let(:mock_config) { instance_double("::Shakapacker::Config") } # rubocop:disable RSpec/VerifiedDoubleReference + + before do + allow(::Shakapacker).to receive(:config).and_return(mock_config) + end + + context "when shakapacker is not defined" do + before do + hide_const("::Shakapacker") + end + + it "returns false" do + expect(described_class.shakapacker_precompile_hook_configured?).to be(false) + end + end + + context "when precompile hook contains react_on_rails:generate_packs" do + it "returns true for direct command with symbol keys" do + allow(mock_config).to receive(:send).with(:data).and_return( + { hooks: { precompile: "bundle exec rake react_on_rails:generate_packs" } } + ) + + expect(described_class.shakapacker_precompile_hook_configured?).to be(true) + end + + it "returns true for direct command with string keys" do + allow(mock_config).to receive(:send).with(:data).and_return( + { "hooks" => { "precompile" => "bundle exec rake react_on_rails:generate_packs" } } + ) + + expect(described_class.shakapacker_precompile_hook_configured?).to be(true) + end + + it "returns true when hook points to script file containing the task" do + allow(mock_config).to receive(:send).with(:data).and_return( + { hooks: { precompile: "bin/shakapacker_precompile" } } + ) + + # Mock Rails.root + rails_root = instance_double(Pathname) + allow(Rails).to receive(:root).and_return(rails_root) + + script_path = instance_double(Pathname, file?: true) + allow(rails_root).to receive(:join).with("bin/shakapacker_precompile").and_return(script_path) + allow(File).to receive(:exist?).with(script_path).and_return(true) + allow(File).to receive(:read).with(script_path) + .and_return("#!/bin/bash\nbundle exec rake react_on_rails:generate_packs\n") + + expect(described_class.shakapacker_precompile_hook_configured?).to be(true) + end + + it "returns false when hook points to script file without the task" do + allow(mock_config).to receive(:send).with(:data).and_return( + { hooks: { precompile: "bin/other_script" } } + ) + + # Mock Rails.root + rails_root = instance_double(Pathname) + allow(Rails).to receive(:root).and_return(rails_root) + + script_path = instance_double(Pathname, file?: true) + allow(rails_root).to receive(:join).with("bin/other_script").and_return(script_path) + allow(File).to receive(:exist?).with(script_path).and_return(true) + allow(File).to receive(:read).with(script_path) + .and_return("#!/bin/bash\necho 'doing other stuff'\n") + + expect(described_class.shakapacker_precompile_hook_configured?).to be(false) + end + end + + context "when precompile hook does not contain react_on_rails:generate_packs" do + it "returns false for different hook" do + allow(mock_config).to receive(:send).with(:data).and_return( + { hooks: { precompile: "bundle exec rake some_other_task" } } + ) + + expect(described_class.shakapacker_precompile_hook_configured?).to be(false) + end + + it "returns false for similar but different task name" do + allow(mock_config).to receive(:send).with(:data).and_return( + { hooks: { precompile: "bundle exec rake react_on_rails:generate_packs_extra" } } + ) + + expect(described_class.shakapacker_precompile_hook_configured?).to be(false) + end + + it "returns false when hooks is nil" do + allow(mock_config).to receive(:send).with(:data).and_return({}) + + expect(described_class.shakapacker_precompile_hook_configured?).to be(false) + end + + it "returns false when precompile hook is nil" do + allow(mock_config).to receive(:send).with(:data).and_return({ hooks: {} }) + + expect(described_class.shakapacker_precompile_hook_configured?).to be(false) + end + end + + context "when an error occurs" do + it "returns false" do + allow(mock_config).to receive(:send).with(:data).and_raise(StandardError.new("test error")) + + expect(described_class.shakapacker_precompile_hook_configured?).to be(false) + end + end + end + + describe ".shakapacker_precompile_hook_value" do + let(:mock_config) { instance_double("::Shakapacker::Config") } # rubocop:disable RSpec/VerifiedDoubleReference + + before do + allow(::Shakapacker).to receive(:config).and_return(mock_config) + end + + it "returns the hook value when configured" do + hook_value = "bin/shakapacker_precompile" + allow(mock_config).to receive(:send).with(:data).and_return( + { hooks: { precompile: hook_value } } + ) + + expect(described_class.shakapacker_precompile_hook_value).to eq(hook_value) + end + + it "returns nil when no hook is configured" do + allow(mock_config).to receive(:send).with(:data).and_return({}) + + expect(described_class.shakapacker_precompile_hook_value).to be_nil + end + + it "returns nil when an error occurs" do + allow(mock_config).to receive(:send).with(:data).and_raise(StandardError.new("test error")) + + expect(described_class.shakapacker_precompile_hook_value).to be_nil + end + end end describe "version constants validation" do @@ -172,3 +312,4 @@ module ReactOnRails end end end +# rubocop:enable Metrics/ModuleLength