Improve precompile hook error reporting and code clarity

justin808 · claude · justin808 · commit 3100a087aee8 · 2025-11-22T11:32:56.000-10:00
This commit enhances the bin/dev precompile hook feature with better error reporting and clearer code documentation based on code review feedback. Key improvements: - Enhanced error reporting: Hook failures now display stdout and stderr output to help users debug issues, using Open3.capture3 instead of system() - Improved documentation: Added comments explaining why SHAKAPACKER_SKIP_PRECOMPILE_HOOK is always set (provides consistent signal for custom scripts) - Better code clarity: Added descriptive variable names in version check (has_precompile_hook_support, has_skip_env_var_support) - Structured changelog: Broke long entry into scannable sub-bullets - Updated tests: All 7 test cases now use Open3.capture3 with proper status mocks - Reduced complexity: Extracted error handling into separate method to fix cyclomatic complexity All tests pass (35 examples, 0 failures) and RuboCop is clean (153 files, no offenses). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,7 +25,12 @@ Changes since the last non-beta release.
 
 #### Improved
 
-- **Automatic Precompile Hook Coordination in bin/dev**: The `bin/dev` command now automatically runs Shakapacker's `precompile_hook` once before starting development processes and sets `SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true` to prevent duplicate execution in spawned webpack processes. This eliminates the need for manual coordination, sleep hacks, and duplicate task calls in Procfile.dev. Users can now configure expensive build tasks (like locale generation or ReScript compilation) once in `config/shakapacker.yml` and `bin/dev` will handle the coordination automatically. The feature includes a warning for users on Shakapacker versions below 9.4.0, as the `SHAKAPACKER_SKIP_PRECOMPILE_HOOK` environment variable is only supported in 9.4.0+. Addresses [2091](https://github.com/shakacode/react_on_rails/issues/2091) by [justin808](https://github.com/justin808).
+- **Automatic Precompile Hook Coordination in bin/dev**: The `bin/dev` command now automatically runs Shakapacker's `precompile_hook` once before starting development processes and sets `SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true` to prevent duplicate execution in spawned webpack processes.
+  - Eliminates the need for manual coordination, sleep hacks, and duplicate task calls in Procfile.dev
+  - Users can configure expensive build tasks (like locale generation or ReScript compilation) once in `config/shakapacker.yml` and `bin/dev` handles coordination automatically
+  - Includes warning for Shakapacker versions below 9.4.0 (the `SHAKAPACKER_SKIP_PRECOMPILE_HOOK` environment variable is only supported in 9.4.0+)
+  - The `SHAKAPACKER_SKIP_PRECOMPILE_HOOK` environment variable is set for all spawned processes, making it available for custom scripts that need to detect when `bin/dev` is managing the precompile hook
+  - Addresses [2091](https://github.com/shakacode/react_on_rails/issues/2091) by [justin808](https://github.com/justin808)
 
 ### [v16.2.0.beta.12] - 2025-11-20
 
diff --git a/lib/react_on_rails/dev/server_manager.rb b/lib/react_on_rails/dev/server_manager.rb
@@ -182,7 +182,10 @@ def run_from_command_line(args = ARGV)
           end.parse!(args)
 
           # Run precompile hook once before starting any mode (except kill/help)
-          # Then set environment variable to prevent duplicate execution in spawned processes
+          # Then set environment variable to prevent duplicate execution in spawned processes.
+          # Note: We always set SHAKAPACKER_SKIP_PRECOMPILE_HOOK=true (even when no hook is configured)
+          # to provide a consistent signal that bin/dev is managing the precompile lifecycle.
+          # This allows custom scripts to detect bin/dev's presence and adjust behavior accordingly.
           unless %w[kill help].include?(command) || help_requested
             run_precompile_hook_if_present
             ENV["SHAKAPACKER_SKIP_PRECOMPILE_HOOK"] = "true"
@@ -211,8 +214,9 @@ def run_from_command_line(args = ARGV)
 
         private
 
-        # rubocop:disable Metrics/AbcSize
         def run_precompile_hook_if_present
+          require "open3"
+
           hook_value = PackerUtils.shakapacker_precompile_hook_value
           return unless hook_value
 
@@ -223,24 +227,51 @@ def run_precompile_hook_if_present
           puts Rainbow("   Command: #{hook_value}").cyan
           puts ""
 
-          unless system(hook_value.to_s)
-            puts ""
-            puts Rainbow("❌ Precompile hook failed!").red.bold
-            puts Rainbow("   Command: #{hook_value}").red
+          # Capture stdout and stderr for better error reporting
+          stdout, stderr, status = Open3.capture3(hook_value.to_s)
+
+          if status.success?
+            puts Rainbow("✅ Precompile hook completed successfully").green
             puts ""
-            puts Rainbow("💡 Fix the hook command in config/shakapacker.yml or remove it to continue").yellow
-            exit 1
+          else
+            handle_precompile_hook_failure(hook_value, stdout, stderr)
           end
+        end
 
-          puts Rainbow("✅ Precompile hook completed successfully").green
+        # rubocop:disable Metrics/AbcSize
+        def handle_precompile_hook_failure(hook_value, stdout, stderr)
+          puts ""
+          puts Rainbow("❌ Precompile hook failed!").red.bold
+          puts Rainbow("   Command: #{hook_value}").red
           puts ""
+
+          if stdout && !stdout.strip.empty?
+            puts Rainbow("   Output:").yellow
+            stdout.strip.split("\n").each { |line| puts Rainbow("   #{line}").yellow }
+            puts ""
+          end
+
+          if stderr && !stderr.strip.empty?
+            puts Rainbow("   Error:").red
+            stderr.strip.split("\n").each { |line| puts Rainbow("   #{line}").red }
+            puts ""
+          end
+
+          puts Rainbow("💡 Fix the hook command in config/shakapacker.yml or remove it to continue").yellow
+          exit 1
         end
         # rubocop:enable Metrics/AbcSize
 
         # rubocop:disable Metrics/AbcSize
         def warn_if_shakapacker_version_too_old
-          return unless PackerUtils.shakapacker_version_requirement_met?("9.0.0")
-          return if PackerUtils.shakapacker_version_requirement_met?("9.4.0")
+          # Only warn for Shakapacker versions in the range 9.0.0 to 9.3.x
+          # Versions below 9.0.0 don't use the precompile_hook feature
+          # Versions 9.4.0+ support SHAKAPACKER_SKIP_PRECOMPILE_HOOK environment variable
+          has_precompile_hook_support = PackerUtils.shakapacker_version_requirement_met?("9.0.0")
+          has_skip_env_var_support = PackerUtils.shakapacker_version_requirement_met?("9.4.0")
+
+          return unless has_precompile_hook_support
+          return if has_skip_env_var_support
 
           puts ""
           puts Rainbow("⚠️  Warning: Shakapacker #{PackerUtils.shakapacker_version} detected").yellow.bold
diff --git a/spec/react_on_rails/dev/server_manager_spec.rb b/spec/react_on_rails/dev/server_manager_spec.rb
@@ -289,21 +289,21 @@ def mock_system_calls
       end
 
       it "runs the hook and sets environment variable for development mode" do
-        expect_any_instance_of(Kernel)
-          .to receive(:system)
+        status_double = instance_double(Process::Status, success?: true)
+        expect(Open3).to receive(:capture3)
           .with("bundle exec rake react_on_rails:locale")
-          .and_return(true)
+          .and_return(["", "", status_double])
 
         described_class.run_from_command_line([])
 
         expect(ENV.fetch("SHAKAPACKER_SKIP_PRECOMPILE_HOOK", nil)).to eq("true")
       end
 
       it "runs the hook and sets environment variable for static mode" do
-        expect_any_instance_of(Kernel)
-          .to receive(:system)
+        status_double = instance_double(Process::Status, success?: true)
+        expect(Open3).to receive(:capture3)
           .with("bundle exec rake react_on_rails:locale")
-          .and_return(true)
+          .and_return(["", "", status_double])
 
         described_class.run_from_command_line(["static"])
 
@@ -313,47 +313,50 @@ def mock_system_calls
       it "runs the hook and sets environment variable for prod mode" do
         env = { "NODE_ENV" => "production" }
         argv = ["bundle", "exec", "rails", "assets:precompile"]
-        status_double = instance_double(Process::Status, success?: true)
-        expect(Open3).to receive(:capture3).with(env, *argv).and_return(["output", "", status_double])
+        assets_status_double = instance_double(Process::Status, success?: true)
+        hook_status_double = instance_double(Process::Status, success?: true)
 
-        expect_any_instance_of(Kernel)
-          .to receive(:system)
+        # Expect both Open3.capture3 calls: one for the hook, one for assets:precompile
+        expect(Open3).to receive(:capture3)
           .with("bundle exec rake react_on_rails:locale")
-          .and_return(true)
+          .and_return(["", "", hook_status_double])
+        expect(Open3).to receive(:capture3)
+          .with(env, *argv)
+          .and_return(["output", "", assets_status_double])
 
         described_class.run_from_command_line(["prod"])
 
         expect(ENV.fetch("SHAKAPACKER_SKIP_PRECOMPILE_HOOK", nil)).to eq("true")
       end
 
       it "exits when hook fails" do
-        expect_any_instance_of(Kernel)
-          .to receive(:system)
+        status_double = instance_double(Process::Status, success?: false)
+        expect(Open3).to receive(:capture3)
           .with("bundle exec rake react_on_rails:locale")
-          .and_return(false)
+          .and_return(["", "", status_double])
         expect_any_instance_of(Kernel).to receive(:exit).with(1)
 
         described_class.run_from_command_line([])
       end
 
       it "does not run hook or set environment variable for kill command" do
-        expect_any_instance_of(Kernel).not_to receive(:system).with("bundle exec rake react_on_rails:locale")
+        expect(Open3).not_to receive(:capture3)
 
         described_class.run_from_command_line(["kill"])
 
         expect(ENV.fetch("SHAKAPACKER_SKIP_PRECOMPILE_HOOK", nil)).to be_nil
       end
 
       it "does not run hook or set environment variable for help command" do
-        expect_any_instance_of(Kernel).not_to receive(:system).with("bundle exec rake react_on_rails:locale")
+        expect(Open3).not_to receive(:capture3)
 
         described_class.run_from_command_line(["help"])
 
         expect(ENV.fetch("SHAKAPACKER_SKIP_PRECOMPILE_HOOK", nil)).to be_nil
       end
 
       it "does not run hook or set environment variable for -h flag" do
-        expect_any_instance_of(Kernel).not_to receive(:system).with("bundle exec rake react_on_rails:locale")
+        expect(Open3).not_to receive(:capture3)
 
         # The -h flag is handled by OptionParser and calls exit during option parsing
         # We need to mock exit to prevent the test from actually exiting
@@ -374,10 +377,10 @@ def mock_system_calls
         end
 
         it "displays warning about unsupported SHAKAPACKER_SKIP_PRECOMPILE_HOOK" do
-          expect_any_instance_of(Kernel)
-            .to receive(:system)
+          status_double = instance_double(Process::Status, success?: true)
+          expect(Open3).to receive(:capture3)
             .with("bundle exec rake react_on_rails:locale")
-            .and_return(true)
+            .and_return(["", "", status_double])
 
           expect do
             described_class.run_from_command_line([])
@@ -397,10 +400,10 @@ def mock_system_calls
         end
 
         it "does not display warning" do
-          expect_any_instance_of(Kernel)
-            .to receive(:system)
+          status_double = instance_double(Process::Status, success?: true)
+          expect(Open3).to receive(:capture3)
             .with("bundle exec rake react_on_rails:locale")
-            .and_return(true)
+            .and_return(["", "", status_double])
 
           expect do
             described_class.run_from_command_line([])
@@ -414,7 +417,9 @@ def mock_system_calls
         allow(ReactOnRails::PackerUtils).to receive(:shakapacker_precompile_hook_value).and_return(nil)
       end
 
-      it "does not run any hook but still sets environment variable for development mode" do
+      it "sets environment variable even when no hook is configured (provides consistent signal)" do
+        # The environment variable is intentionally set even when no hook exists
+        # to provide a consistent signal that bin/dev is managing the precompile lifecycle
         expect_any_instance_of(Kernel).not_to receive(:system)
 
         described_class.run_from_command_line([])
