Skip to content

Fix load-based precompile hook execution path #2419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
justin808 merged 3 commits into from
Mar 8, 2026
Merged

Fix load-based precompile hook execution path #2419

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7b8eaef
Make shared precompile hook explicitly runnable after load
justin808 Feb 15, 2026
3fe8113
Use modifier if for precompile hook entrypoint
justin808 Feb 15, 2026
cb745f5
Retrigger Claude Code Review
justin808 Feb 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions react_on_rails/spec/dummy/bin/shakapacker-precompile-hook
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@ unless File.exist?(shared_hook)
exit 1
end

# Load shared implementation (defines build_rescript_if_needed and generate_packs_if_needed)
# Load shared implementation (defines run_precompile_tasks)
load shared_hook

# Explicitly invoke the functions. The shared file's `if __FILE__ == $PROGRAM_NAME`
# block does not run when the file is loaded via `load`.
build_rescript_if_needed
generate_packs_if_needed
# Explicitly invoke run_precompile_tasks. The shared file's
# `if __FILE__ == $PROGRAM_NAME` block does not run when loaded via `load`.
run_precompile_tasks
19 changes: 19 additions & 0 deletions react_on_rails/spec/react_on_rails/shakapacker_precompile_hook_shared_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

require_relative "spec_helper"

RSpec.describe "Shakapacker precompile hook shared script" do
before do
load File.expand_path("../support/shakapacker_precompile_hook_shared.rb", __dir__)
end

it "exposes run_precompile_tasks for load-based callers" do
allow(self).to receive(:build_rescript_if_needed)
allow(self).to receive(:generate_packs_if_needed)

run_precompile_tasks

expect(self).to have_received(:build_rescript_if_needed)
expect(self).to have_received(:generate_packs_if_needed)
end
Comment on lines +6 to +18

@greptile-apps greptile-apps Bot Feb 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global method pollution from load in before block

Using load in the before block defines run_precompile_tasks, build_rescript_if_needed, generate_packs_if_needed, find_rails_root, and detect_package_manager as private instance methods on Object. These persist for the entire test suite run and could interfere with other specs.

Consider wrapping the load in a clean module to isolate these methods:

Suggested change
before do
load File.expand_path("../support/shakapacker_precompile_hook_shared.rb", __dir__)
end
it "exposes run_precompile_tasks for load-based callers" do
allow(self).to receive(:build_rescript_if_needed)
allow(self).to receive(:generate_packs_if_needed)
run_precompile_tasks
expect(self).to have_received(:build_rescript_if_needed)
expect(self).to have_received(:generate_packs_if_needed)
end
let(:hook_module) do
Module.new.tap { |mod| mod.module_eval(File.read(File.expand_path("../support/shakapacker_precompile_hook_shared.rb", __dir__))) }
end
it "exposes run_precompile_tasks for load-based callers" do
allow(hook_module).to receive(:build_rescript_if_needed)
allow(hook_module).to receive(:generate_packs_if_needed)
hook_module.run_precompile_tasks
expect(hook_module).to have_received(:build_rescript_if_needed)
expect(hook_module).to have_received(:generate_packs_if_needed)
end

Alternatively, at minimum, the before block should use load shared_path, true (the second argument wraps the code in an anonymous module) to prevent global leakage:

before do
  load File.expand_path("../support/shakapacker_precompile_hook_shared.rb", __dir__), true
end

Note: using load path, true would wrap the methods in an anonymous module, making them inaccessible from the test. So the Module.new approach or a different isolation strategy would be needed if this is a concern.

end
Comment on lines +1 to +19

@greptile-apps greptile-apps Bot Feb 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test was not run per PR description

The PR description states this test is "UNTESTED in this environment" due to a Ruby version mismatch. Per AGENTS.md policy: "Never claim a test is 'fixed' without running it locally first." The test should be verified in CI or by another contributor before merging.

Additionally, the test only asserts that run_precompile_tasks delegates to the two sub-methods — something trivially obvious from the 3-line implementation. It doesn't test the actual load-based execution path from the hook wrappers (i.e., that load shared_hook followed by run_precompile_tasks works end-to-end). A more meaningful test would invoke the dummy bin/shakapacker-precompile-hook script in a subprocess and verify the expected output or side effects.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

6 changes: 5 additions & 1 deletion react_on_rails/spec/support/shakapacker_precompile_hook_shared.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,11 @@ def generate_packs_if_needed
end

# Main execution (only if run directly, not when required)
if __FILE__ == $PROGRAM_NAME
def run_precompile_tasks
build_rescript_if_needed
generate_packs_if_needed
end

if __FILE__ == $PROGRAM_NAME
run_precompile_tasks
end
9 changes: 4 additions & 5 deletions react_on_rails_pro/spec/dummy/bin/shakapacker-precompile-hook
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@ unless File.exist?(shared_hook)
exit 1
end

# Load shared implementation (defines build_rescript_if_needed and generate_packs_if_needed)
# Load shared implementation (defines run_precompile_tasks)
load shared_hook

# Explicitly invoke the functions. The shared file's `if __FILE__ == $PROGRAM_NAME`
# block does not run when the file is loaded via `load`.
build_rescript_if_needed
generate_packs_if_needed
# Explicitly invoke run_precompile_tasks. The shared file's
# `if __FILE__ == $PROGRAM_NAME` block does not run when loaded via `load`.
run_precompile_tasks
Loading