-
-
Notifications
You must be signed in to change notification settings - Fork 634
Fix first-start bin/dev precompile hook detection for RSC demo #2561
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,10 +1,15 @@ | ||||||||||||
| # frozen_string_literal: true | ||||||||||||
|
|
||||||||||||
| require "erb" | ||||||||||||
| require "pathname" | ||||||||||||
| require "shakapacker" | ||||||||||||
| require "yaml" | ||||||||||||
|
|
||||||||||||
| module ReactOnRails | ||||||||||||
| # rubocop:disable Metrics/ModuleLength | ||||||||||||
| module PackerUtils | ||||||||||||
| SHAKAPACKER_CONFIG_PATH = File.join("config", "shakapacker.yml") | ||||||||||||
|
|
||||||||||||
| def self.dev_server_running? | ||||||||||||
| Shakapacker.dev_server.running? | ||||||||||||
| end | ||||||||||||
|
|
@@ -180,21 +185,20 @@ def self.shakapacker_precompile_hook_configured? | |||||||||||
| end | ||||||||||||
|
|
||||||||||||
| def self.extract_precompile_hook | ||||||||||||
| # Prefer the public API (available in Shakapacker 9.0+) | ||||||||||||
| return ::Shakapacker.config.precompile_hook if ::Shakapacker.config.respond_to?(:precompile_hook) | ||||||||||||
|
|
||||||||||||
| # Fallback: access config data using private :data method | ||||||||||||
| config_data = ::Shakapacker.config.send(:data) | ||||||||||||
| # Prefer using Shakapacker's runtime config when available. | ||||||||||||
| # In bin/dev startup before Rails boots, this can raise (e.g., missing Rails.env), | ||||||||||||
| # so we rescue and fall back to parsing config/shakapacker.yml directly. | ||||||||||||
| hook_value = extract_precompile_hook_from_shakapacker_config | ||||||||||||
| return hook_value unless hook_value.nil? | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The fallback to YAML parsing is triggered whenever Consider distinguishing a "failed to load config" nil from a "config loaded, no hook present" nil, e.g. with a sentinel: # In extract_precompile_hook_from_shakapacker_config, return a sentinel on success-but-no-hook
HOOK_NOT_CONFIGURED = :hook_not_configured
# Then in extract_precompile_hook:
hook_value = extract_precompile_hook_from_shakapacker_config
return nil if hook_value == HOOK_NOT_CONFIGURED
return hook_value unless hook_value.nil?
extract_precompile_hook_from_yamlAlternatively, have |
||||||||||||
|
|
||||||||||||
| # Try symbol keys first (Shakapacker's internal format), then fall back to string keys | ||||||||||||
| config_data&.[](:precompile_hook) || config_data&.[]("precompile_hook") | ||||||||||||
| extract_precompile_hook_from_yaml | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| # Regex pattern to detect pack generation in hook scripts | ||||||||||||
| # Matches both: | ||||||||||||
| # - The rake task: react_on_rails:generate_packs | ||||||||||||
| # - The Ruby method: generate_packs_if_stale (used by generator template) | ||||||||||||
| GENERATE_PACKS_PATTERN = /\b(react_on_rails:generate_packs|generate_packs_if_stale)\b/ | ||||||||||||
| # - The Ruby methods: generate_packs_if_stale / generate_packs_if_needed | ||||||||||||
| GENERATE_PACKS_PATTERN = /\b(react_on_rails:generate_packs|generate_packs_if_stale|generate_packs_if_needed)\b/ | ||||||||||||
|
|
||||||||||||
| # Pattern to detect a real self-guard statement that exits early when | ||||||||||||
| # SHAKAPACKER_SKIP_PRECOMPILE_HOOK is true. This avoids false positives | ||||||||||||
|
|
@@ -217,7 +221,7 @@ def self.hook_contains_generate_packs?(hook_value) | |||||||||||
|
|
||||||||||||
| # 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) | ||||||||||||
| return false unless script_path | ||||||||||||
|
|
||||||||||||
| # Read and check script contents | ||||||||||||
| script_contents = File.read(script_path) | ||||||||||||
|
|
@@ -230,20 +234,29 @@ def self.hook_contains_generate_packs?(hook_value) | |||||||||||
| def self.resolve_hook_script_path(hook_value) | ||||||||||||
| return nil if hook_value.blank? | ||||||||||||
|
|
||||||||||||
| potential_path = project_root.join(hook_value.to_s.strip) | ||||||||||||
| hook_path = hook_value.to_s.strip | ||||||||||||
| return nil if hook_path.empty? | ||||||||||||
|
|
||||||||||||
| # Strip interpreter prefix (e.g., "ruby bin/hook" -> "bin/hook") | ||||||||||||
| hook_path = extract_script_from_command(hook_path) || hook_path | ||||||||||||
|
|
||||||||||||
| # Hook value might be a script path relative to project root. | ||||||||||||
| # project_root prefers Rails.root and otherwise derives from BUNDLE_GEMFILE or cwd. | ||||||||||||
| potential_path = project_root.join(hook_path) | ||||||||||||
| potential_path if potential_path.file? | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| def self.project_root | ||||||||||||
| return Rails.root if defined?(Rails) && Rails.respond_to?(:root) && Rails.root | ||||||||||||
| # Extract the script path from an interpreter-prefixed command. | ||||||||||||
| # e.g., "ruby bin/shakapacker-precompile-hook" -> "bin/shakapacker-precompile-hook" | ||||||||||||
| # Returns nil if the value doesn't look like an interpreter-prefixed command. | ||||||||||||
| def self.extract_script_from_command(command) | ||||||||||||
| parts = command.strip.split(/\s+/, 2) | ||||||||||||
| return nil unless parts.length == 2 | ||||||||||||
|
|
||||||||||||
| bundle_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) | ||||||||||||
| if bundle_gemfile && !bundle_gemfile.strip.empty? | ||||||||||||
| gemfile_path = Pathname.new(bundle_gemfile).expand_path | ||||||||||||
| return gemfile_path.dirname if gemfile_path.file? | ||||||||||||
| end | ||||||||||||
| interpreter = File.basename(parts[0]) | ||||||||||||
| return parts[1] if %w[ruby node bash sh].include?(interpreter) | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The hard-coded interpreter list only covers
The If |
||||||||||||
|
|
||||||||||||
| Pathname.new(Dir.pwd) | ||||||||||||
| nil | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| # Check if a hook script file contains the self-guard pattern that prevents | ||||||||||||
|
|
@@ -270,5 +283,79 @@ def self.shakapacker_precompile_hook_value | |||||||||||
| rescue StandardError | ||||||||||||
| nil | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| def self.extract_precompile_hook_from_shakapacker_config | ||||||||||||
| # Prefer the public API (available in Shakapacker 9.0+) | ||||||||||||
| return ::Shakapacker.config.precompile_hook if ::Shakapacker.config.respond_to?(:precompile_hook) | ||||||||||||
|
|
||||||||||||
| # Fallback: access config data using private :data method | ||||||||||||
| config_data = ::Shakapacker.config.send(:data) | ||||||||||||
|
|
||||||||||||
| # Try symbol keys first (Shakapacker's internal format), then fall back to string keys | ||||||||||||
| if config_data&.key?(:precompile_hook) | ||||||||||||
| config_data[:precompile_hook] | ||||||||||||
| elsif config_data&.key?("precompile_hook") | ||||||||||||
| config_data["precompile_hook"] | ||||||||||||
| end | ||||||||||||
| rescue StandardError | ||||||||||||
| nil | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| def self.extract_precompile_hook_from_yaml | ||||||||||||
| config_path = project_root.join(SHAKAPACKER_CONFIG_PATH) | ||||||||||||
| return nil unless config_path.file? | ||||||||||||
|
|
||||||||||||
| yaml_content = ERB.new(File.read(config_path)).result | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ERB evaluation is consistent with how Rails and Shakapacker themselves load this file, so this is acceptable. Worth adding a comment here documenting the intent so future readers aren't alarmed:
Suggested change
|
||||||||||||
| config_data = YAML.safe_load(yaml_content, permitted_classes: [Symbol], aliases: true) || {} | ||||||||||||
|
|
||||||||||||
| env_config = extract_hash_for_environment(config_data, current_shakapacker_environment) | ||||||||||||
| return env_config["precompile_hook"] if env_config.key?("precompile_hook") | ||||||||||||
| return env_config[:precompile_hook] if env_config.key?(:precompile_hook) | ||||||||||||
|
|
||||||||||||
| default_config = extract_hash_for_environment(config_data, "default") | ||||||||||||
| return default_config["precompile_hook"] if default_config.key?("precompile_hook") | ||||||||||||
| return default_config[:precompile_hook] if default_config.key?(:precompile_hook) | ||||||||||||
|
|
||||||||||||
| nil | ||||||||||||
| rescue StandardError | ||||||||||||
| nil | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| def self.extract_hash_for_environment(config_data, env_name) | ||||||||||||
| value = config_data[env_name] || config_data[env_name.to_sym] | ||||||||||||
| value.is_a?(Hash) ? value : {} | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| def self.current_shakapacker_environment | ||||||||||||
| if defined?(Rails) && Rails.respond_to?(:env) | ||||||||||||
| env = begin | ||||||||||||
| Rails.env.to_s | ||||||||||||
| rescue StandardError | ||||||||||||
| nil | ||||||||||||
| end | ||||||||||||
| return env unless env.to_s.strip.empty? | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| rails_env = ENV.fetch("RAILS_ENV", nil) | ||||||||||||
| return rails_env unless rails_env.to_s.strip.empty? | ||||||||||||
|
|
||||||||||||
| rack_env = ENV.fetch("RACK_ENV", nil) | ||||||||||||
| return rack_env unless rack_env.to_s.strip.empty? | ||||||||||||
|
|
||||||||||||
| "development" | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| def self.project_root | ||||||||||||
| return Rails.root if defined?(Rails) && Rails.respond_to?(:root) && Rails.root | ||||||||||||
|
|
||||||||||||
| bundle_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) | ||||||||||||
| if bundle_gemfile && !bundle_gemfile.strip.empty? | ||||||||||||
| gemfile_path = Pathname.new(bundle_gemfile).expand_path | ||||||||||||
| return gemfile_path.dirname if gemfile_path.file? | ||||||||||||
| end | ||||||||||||
|
|
||||||||||||
| Pathname.new(Dir.pwd) | ||||||||||||
| end | ||||||||||||
| end | ||||||||||||
| # rubocop:enable Metrics/ModuleLength | ||||||||||||
| end | ||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||||||||
| # frozen_string_literal: true | ||||||||||
|
|
||||||||||
| require_relative "spec_helper" | ||||||||||
| require "fileutils" | ||||||||||
| require "tmpdir" | ||||||||||
|
|
||||||||||
| # rubocop:disable Metrics/ModuleLength | ||||||||||
| module ReactOnRails | ||||||||||
|
|
@@ -156,6 +158,28 @@ module ReactOnRails | |||||||||
|
|
||||||||||
| expect(described_class.extract_precompile_hook).to eq(hook_value) | ||||||||||
| end | ||||||||||
|
|
||||||||||
| it "falls back to config/shakapacker.yml when Shakapacker config is unavailable pre-Rails" do | ||||||||||
| Dir.mktmpdir do |tmp_dir| | ||||||||||
| config_dir = File.join(tmp_dir, "config") | ||||||||||
| FileUtils.mkdir_p(config_dir) | ||||||||||
| File.write(File.join(tmp_dir, "Gemfile"), "source 'https://rubygems.org'\n") | ||||||||||
| File.write(File.join(config_dir, "shakapacker.yml"), <<~YAML) | ||||||||||
| default: &default | ||||||||||
| precompile_hook: 'bin/shakapacker-precompile-hook' | ||||||||||
| development: | ||||||||||
| <<: *default | ||||||||||
| YAML | ||||||||||
|
|
||||||||||
| stub_const("ENV", ENV.to_h.merge("BUNDLE_GEMFILE" => File.join(tmp_dir, "Gemfile"))) | ||||||||||
|
|
||||||||||
| hide_const("Rails") | ||||||||||
| allow(::Shakapacker).to receive(:config) | ||||||||||
| .and_raise(NameError, "uninitialized constant Shakapacker::Env::Rails") | ||||||||||
|
|
||||||||||
| expect(described_class.extract_precompile_hook).to eq("bin/shakapacker-precompile-hook") | ||||||||||
| end | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
| describe ".shakapacker_precompile_hook_configured?" do | ||||||||||
|
|
@@ -224,6 +248,18 @@ module ReactOnRails | |||||||||
| expect(described_class.shakapacker_precompile_hook_configured?).to be true | ||||||||||
| end | ||||||||||
|
|
||||||||||
| it "returns true when script contains generate_packs_if_needed helper" do | ||||||||||
| allow(script_full_path).to receive(:file?).and_return(true) | ||||||||||
| allow(File).to receive(:exist?).with(script_full_path).and_return(true) | ||||||||||
| allow(File).to receive(:read).with(script_full_path).and_return(<<~RUBY) | ||||||||||
| #!/usr/bin/env ruby | ||||||||||
| load "spec/support/shakapacker_precompile_hook_shared.rb" | ||||||||||
| generate_packs_if_needed | ||||||||||
| RUBY | ||||||||||
|
|
||||||||||
| expect(described_class.shakapacker_precompile_hook_configured?).to be true | ||||||||||
| end | ||||||||||
|
|
||||||||||
| it "returns false when script doesn't contain generate_packs" do | ||||||||||
| allow(script_full_path).to receive(:file?).and_return(true) | ||||||||||
| allow(File).to receive(:exist?).with(script_full_path).and_return(true) | ||||||||||
|
|
@@ -242,6 +278,42 @@ module ReactOnRails | |||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
| context "when Rails is unavailable during early bin/dev startup" do | ||||||||||
| it "detects script-based hooks from shakapacker.yml fallback" do | ||||||||||
| Dir.mktmpdir do |tmp_dir| | ||||||||||
| config_dir = File.join(tmp_dir, "config") | ||||||||||
| bin_dir = File.join(tmp_dir, "bin") | ||||||||||
| FileUtils.mkdir_p(config_dir) | ||||||||||
| FileUtils.mkdir_p(bin_dir) | ||||||||||
|
|
||||||||||
| File.write(File.join(tmp_dir, "Gemfile"), "source 'https://rubygems.org'\n") | ||||||||||
| File.write(File.join(config_dir, "shakapacker.yml"), <<~YAML) | ||||||||||
| default: &default | ||||||||||
| precompile_hook: 'bin/shakapacker-precompile-hook' | ||||||||||
| development: | ||||||||||
| <<: *default | ||||||||||
| YAML | ||||||||||
|
|
||||||||||
| File.write(File.join(bin_dir, "shakapacker-precompile-hook"), <<~RUBY) | ||||||||||
| #!/usr/bin/env ruby | ||||||||||
| ReactOnRails::PacksGenerator.instance.generate_packs_if_stale | ||||||||||
| RUBY | ||||||||||
|
|
||||||||||
| old_bundle_gemfile = ENV.fetch("BUNDLE_GEMFILE", nil) | ||||||||||
| ENV["BUNDLE_GEMFILE"] = File.join(tmp_dir, "Gemfile") | ||||||||||
|
Comment on lines
+302
to
+303
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test directly mutates Prefer
Suggested change
|
||||||||||
|
|
||||||||||
| hide_const("Rails") | ||||||||||
| allow(::Shakapacker).to receive(:config) | ||||||||||
| .and_raise(NameError, "uninitialized constant Shakapacker::Env::Rails") | ||||||||||
|
|
||||||||||
| expect(described_class.shakapacker_precompile_hook_value).to eq("bin/shakapacker-precompile-hook") | ||||||||||
| expect(described_class.shakapacker_precompile_hook_configured?).to be true | ||||||||||
| ensure | ||||||||||
| ENV["BUNDLE_GEMFILE"] = old_bundle_gemfile | ||||||||||
| end | ||||||||||
| end | ||||||||||
| end | ||||||||||
|
|
||||||||||
| context "when precompile_hook is not configured" do | ||||||||||
| it "returns false for nil" do | ||||||||||
| allow(mock_config).to receive(:send).with(:data).and_return({ precompile_hook: nil }) | ||||||||||
|
|
||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nil-check here conflates two distinct nil cases:
precompile_hookkey is simply not set → also returns nil → falls through to YAML unnecessarilyIn case 2 the YAML parse produces the same nil, so it is functionally correct, but it means every call where the hook is unconfigured will redundantly parse and ERB-evaluate the YAML file.
Consider tracking whether the runtime call succeeded and only falling back on failure:
Then
extract_precompile_hook_from_shakapacker_configreturns[value, true]on success and[nil, false]on rescue. This makes the intent explicit and avoids the redundant YAML parse.