Skip to content

Commit 00202ba

Browse files
justin808claude
andauthored
refactor: consolidate Shakapacker bundler/config diagnostics helpers (#3619)
## Summary Follow-up cleanup from #3508 (issue #3538). `Doctor`, `SystemChecker`, and `Dev::ServerManager` each carried near-identical copies of the `shakapacker.yml` parsing, assets-bundler detection, bundler labels, and dev-server reload-mode helpers. This consolidates them into one mixin and clears the remaining review nits. ## Changes (mapped to the #3538 checklist) - **Extract shared helpers** — new `ReactOnRails::ShakapackerConfigHelpers`, `include`d by `Doctor`/`SystemChecker` and `extend`ed onto `Dev::ServerManager` (its commands live on `class << self`). Mirrors the existing untyped `ConfigPathResolver` mixin precedent. - **Unify `YAML.safe_load` options** — `SystemChecker` now permits `Symbol` values and rescues `ScriptError`, matching the other parsers and `ServerMode`. - **Memoize parsed config** — `parsed_shakapacker_config` is memoized per `Doctor` instance (a fresh `Doctor` runs per invocation), so one diagnosis reads/parses the file once. `ServerManager` intentionally does **not** memoize — its `class << self` state would leak across specs, the same reason `default_dev_server_mode` isn't memoized. - **Remove the `dev_server_procfile_label` alias** — it was a pure wrapper for `dev_server_label`. - **Remove unused `ServerManager` helpers** — `development_reload_feature_label`, `development_mode_title`, and `hmr_procfile_description` were defined but never called. - **Non-HMR React Refresh help indentation** — the rspack config-hint bullet now lines up with the bullets above; webpack omits the bullet instead of leaving a trailing blank line. - **Babel qualifier decision** — documented that the `react-refresh/babel` `WEBPACK_SERVE` qualifier is intentionally bundler-agnostic (the generated `babel.config.js` gates it on `WEBPACK_SERVE` for both webpack and rspack); only the bundler plugin line is bundler-specific. Net **−109 lines** across the three diagnostics files. ## Testing - `bundle exec rspec` for `system_checker_spec`, `doctor_spec`, `server_manager_spec` → **571 examples, 0 failures** - `bundle exec rubocop` on all four files → **no offenses** - `bundle exec rake rbs:validate` → **passes**. The RBS-declared `ServerManager` API is unchanged (all 28 declared methods still present with the same signatures), so runtime RBS type checking is unaffected. The moved private helpers were never in the RBS, and the new mixin follows the existing untyped `ConfigPathResolver` convention (no `.rbs`, not in the Steepfile; Steep remains disabled in CI). No CHANGELOG entry, per the repo's changelog guidelines (refactors are excluded). Closes #3538 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Refactor-only with no public API changes; behavior should match prior diagnostics/CLI wording aside from intentional YAML/parser alignment and help formatting fixes. > > **Overview** > Introduces **`ReactOnRails::ShakapackerConfigHelpers`** so **`Doctor`**, **`SystemChecker`**, and **`Dev::ServerManager`** share one implementation for reading **`config/shakapacker.yml`**, resolving **`SHAKAPACKER_CONFIG`**, detecting **webpack vs rspack**, and labeling dev-server / HMR vs live reload. > > **`Doctor`** and **`SystemChecker`** **`include`** the mixin; **`ServerManager`** **`extend`**s it so helpers stay on **`class << self`**. Duplicate private methods are removed from all three (~109 net lines). **`SystemChecker`** now uses the same **`YAML.safe_load`** options (**`Symbol`**, **`ScriptError`**) as the other parsers. **`Doctor`** memoizes **`parsed_shakapacker_config`** per run; **`ServerManager`** does not, to avoid class-level state leaking in specs. > > Smaller cleanups: drop unused **`dev_server_procfile_label`** and dead **`ServerManager`** label helpers; fix non-HMR React Refresh help so rspack hints align with bullets (no extra blank line for webpack); document that the **`react-refresh/babel`** **`WEBPACK_SERVE`** note stays bundler-agnostic. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 9f5454f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Refactoring** * Consolidated shakapacker configuration utilities into a dedicated helper module for improved code organization and maintainability across multiple internal classes. * **Bug Fixes** * Fixed React Refresh troubleshooting help text formatting to eliminate unnecessary blank lines in certain bundler configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 9fedd96 commit 00202ba

4 files changed

Lines changed: 162 additions & 271 deletions

File tree

react_on_rails/lib/react_on_rails/dev/server_manager.rb

Lines changed: 16 additions & 117 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
require "uri"
1414
require "yaml"
1515
require_relative "../packer_utils"
16+
require_relative "../shakapacker_config_helpers"
1617
require_relative "../system_checker"
1718
require_relative "database_checker"
1819
require_relative "server_mode"
@@ -21,6 +22,10 @@
2122
module ReactOnRails
2223
module Dev
2324
class ServerManager
25+
# Commands live on `class << self`, so extend (rather than include) the
26+
# shared shakapacker-config helpers to expose them as singleton methods.
27+
extend ReactOnRails::ShakapackerConfigHelpers
28+
2429
HELP_FLAGS = ["-h", "--help"].freeze
2530
TEST_WATCH_MODES = %w[auto full client-only].freeze
2631
OPEN_BROWSER_WAIT_TIMEOUT = 60
@@ -472,53 +477,6 @@ def shared_private_output_paths?
472477
development_private == test_private
473478
end
474479

475-
def parsed_shakapacker_config
476-
config_path = shakapacker_config_path
477-
return nil unless File.exist?(config_path)
478-
479-
YAML.safe_load(ERB.new(File.read(config_path)).result, aliases: true, permitted_classes: [Symbol])
480-
rescue StandardError, ScriptError
481-
nil
482-
end
483-
484-
def configured_assets_bundler
485-
config = parsed_shakapacker_config
486-
return nil unless config.is_a?(Hash)
487-
488-
rails_env = ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "development"
489-
bundler_from_shakapacker_section(config, rails_env) || bundler_from_shakapacker_section(config, "default")
490-
rescue StandardError, ScriptError
491-
nil
492-
end
493-
494-
def active_assets_bundler
495-
configured_assets_bundler || "webpack"
496-
end
497-
498-
def assets_bundler_label
499-
active_assets_bundler.capitalize
500-
end
501-
502-
def dev_server_label
503-
active_assets_bundler == "webpack" ? "webpack-dev-server" : "#{assets_bundler_label} dev server"
504-
end
505-
506-
def development_reload_mode_label
507-
development_hmr_enabled? ? "HMR" : "Live reload"
508-
end
509-
510-
def development_reload_feature_label
511-
development_hmr_enabled? ? "Hot Module Replacement (HMR)" : "Live reload"
512-
end
513-
514-
def development_mode_title
515-
development_hmr_enabled? ? "HMR Development mode (default)" : "Live reload development mode (default)"
516-
end
517-
518-
def hmr_procfile_description
519-
"#{development_reload_mode_label} development with #{dev_server_label}"
520-
end
521-
522480
def default_procfile_description(default_mode)
523481
bundler_aware_dev_server_text(ServerMode.text(default_mode, :procfile_description))
524482
end
@@ -572,74 +530,6 @@ def compilation_failed_label
572530
end
573531
end
574532

575-
def bundler_from_shakapacker_section(config, section_name)
576-
section = config[section_name] || config[section_name.to_sym]
577-
return nil unless section.is_a?(Hash)
578-
579-
normalize_assets_bundler(section["assets_bundler"] || section[:assets_bundler])
580-
end
581-
582-
def normalize_assets_bundler(value)
583-
normalized = value.to_s.strip.downcase
584-
ReactOnRails::SystemChecker::SUPPORTED_ASSETS_BUNDLERS.include?(normalized) ? normalized : nil
585-
end
586-
587-
def development_hmr_enabled?
588-
dev_server = development_dev_server_config
589-
return hmr_config_value?(dev_server["hmr"]) if dev_server.key?("hmr")
590-
591-
return false if truthy_config_value?(dev_server["live_reload"])
592-
593-
# Default to HMR when neither hmr nor live_reload is configured, preserving historical behavior.
594-
true
595-
end
596-
597-
def hmr_config_value?(value)
598-
value.to_s.strip.casecmp?("only") || truthy_config_value?(value)
599-
end
600-
601-
def development_dev_server_config
602-
config = parsed_shakapacker_config
603-
return {} unless config.is_a?(Hash)
604-
605-
development_config = shakapacker_section(config, "default").merge(shakapacker_section(config, "development"))
606-
dev_server_config_for(development_config)
607-
end
608-
609-
def shakapacker_section(config, section_name)
610-
section = config[section_name] || config[section_name.to_sym]
611-
section.is_a?(Hash) ? section : {}
612-
end
613-
614-
def dev_server_config_for(section)
615-
dev_server = section["dev_server"] || section[:dev_server]
616-
return {} unless dev_server.is_a?(Hash)
617-
618-
dev_server.transform_keys(&:to_s)
619-
end
620-
621-
def truthy_config_value?(value)
622-
value == true || value.to_s == "true"
623-
end
624-
625-
# Resolves SHAKAPACKER_CONFIG the same way ReactOnRails::Engine does, so this CLI
626-
# sees the same config file as Rails boot even when invoked from a directory other
627-
# than the Rails root. Falls back to Dir.pwd when Rails isn't loaded (bin/dev does
628-
# not require Rails directly).
629-
def shakapacker_config_path
630-
env_config_path = ENV.fetch("SHAKAPACKER_CONFIG", nil)
631-
base = shakapacker_config_base_dir
632-
return File.expand_path("config/shakapacker.yml", base) if env_config_path.to_s.empty?
633-
634-
File.expand_path(env_config_path, base)
635-
end
636-
637-
def shakapacker_config_base_dir
638-
return Rails.root.to_s if defined?(Rails) && Rails.respond_to?(:root) && Rails.root
639-
640-
Dir.pwd
641-
end
642-
643533
# rubocop:disable Metrics/AbcSize
644534
def handle_precompile_hook_failure(hook_value, stdout, stderr)
645535
puts ""
@@ -893,13 +783,18 @@ def default_dev_server_detail_lines(mode)
893783
def help_react_refresh_troubleshooting(default_mode)
894784
return help_hmr_react_refresh_troubleshooting if default_mode == :hmr
895785

896-
<<~REFRESH
786+
troubleshooting = <<~REFRESH
897787
#{Rainbow('⚛️ React Refresh:').yellow.bold}
898788
#{Rainbow('React Refresh requires HMR; current default mode is not HMR.').white}
899789
#{Rainbow('•').yellow} #{Rainbow(ServerMode.text(default_mode, :refresh_guidance)).white}
900790
#{Rainbow('•').yellow} #{Rainbow(ServerMode.text(default_mode, :refresh_note)).white}
901-
#{rspack_react_refresh_config_hint}
902791
REFRESH
792+
793+
# Append the rspack/other-bundler config-hint bullet only when present so it
794+
# lines up with the bullets above; webpack returns "" and we skip it rather
795+
# than emitting a trailing blank line.
796+
hint = rspack_react_refresh_config_hint
797+
hint.empty? ? troubleshooting : "#{troubleshooting}#{hint}\n"
903798
end
904799

905800
def rspack_react_refresh_config_hint
@@ -913,6 +808,10 @@ def rspack_react_refresh_config_hint
913808
def help_hmr_react_refresh_troubleshooting
914809
plugin_check = "Check that both babel plugin and #{react_refresh_bundler_plugin_description} are configured:"
915810

811+
# The babel `react-refresh/babel` plugin is gated on WEBPACK_SERVE in the
812+
# generated babel.config.js for both webpack and rspack apps, so that
813+
# qualifier intentionally stays bundler-agnostic. Only the bundler plugin
814+
# line below (react_refresh_bundler_config_hint) is bundler-specific.
916815
<<~REFRESH
917816
#{Rainbow('⚛️ React Refresh Issues:').yellow.bold}
918817
#{Rainbow('If you see "$RefreshSig$ is not defined" errors:').white}

react_on_rails/lib/react_on_rails/doctor.rb

Lines changed: 12 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
require "yaml"
88
require_relative "utils"
99
require_relative "config_path_resolver"
10+
require_relative "shakapacker_config_helpers"
1011
require_relative "dev/server_mode"
1112
require_relative "version_syntax_converter"
1213
require_relative "version_synchronizer"
@@ -47,6 +48,7 @@ module ReactOnRails
4748
# rubocop:disable Metrics/ClassLength, Metrics/AbcSize
4849
class Doctor
4950
include ConfigPathResolver
51+
include ShakapackerConfigHelpers
5052

5153
MESSAGE_COLORS = {
5254
error: :red,
@@ -58,7 +60,6 @@ class Doctor
5860
RSPEC_HELPER_FILES = ["spec/rails_helper.rb", "spec/spec_helper.rb"].freeze
5961
MINITEST_HELPER_FILE = "test/test_helper.rb"
6062
DEFAULT_BUILD_TEST_COMMAND = 'config.build_test_command = "RAILS_ENV=test bin/shakapacker"'
61-
DEFAULT_SHAKAPACKER_CONFIG_PATH = "config/shakapacker.yml"
6263
SERVER_BUNDLE_SOURCE_EXTENSIONS = %w[.js .jsx .ts .tsx .mjs .cjs].freeze
6364
CUSTOM_LAUNCHER_INDICATOR_FILES = %w[dev].freeze
6465
RAILS_SERVER_COMMAND_REGEX = %r{\b(?:(?:bin/)?rails\s+(?:server|s)|puma|unicorn|rackup|passenger\s+start)\b}
@@ -341,7 +342,7 @@ def check_individual_procfile(filename, config)
341342

342343
def procfile_descriptions
343344
{
344-
hmr: "#{development_reload_mode_label} development with #{dev_server_procfile_label}",
345+
hmr: "#{development_reload_mode_label} development with #{dev_server_label}",
345346
static: "Static development with #{static_watch_label}"
346347
}
347348
end
@@ -353,7 +354,7 @@ def default_procfile_description(default_mode)
353354
def bundler_aware_dev_server_text(text)
354355
return text if active_assets_bundler == "webpack"
355356

356-
text.gsub("webpack-dev-server", dev_server_procfile_label)
357+
text.gsub("webpack-dev-server", dev_server_label)
357358
end
358359

359360
def check_bin_dev_script
@@ -549,100 +550,22 @@ def print_next_steps
549550
puts
550551
end
551552

552-
def dev_server_label
553-
active_assets_bundler == "webpack" ? "webpack-dev-server" : "#{assets_bundler_label} dev server"
554-
end
555-
556-
def active_assets_bundler
557-
configured_assets_bundler || "webpack"
558-
end
559-
560-
def assets_bundler_label
561-
active_assets_bundler.capitalize
562-
end
563-
564-
def dev_server_procfile_label
565-
dev_server_label
566-
end
567-
568553
def static_watch_label
569554
active_assets_bundler == "webpack" ? "webpack --watch" : "#{active_assets_bundler} watch"
570555
end
556+
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
571557

572-
def development_reload_mode_label
573-
development_hmr_enabled? ? "HMR" : "Live reload"
574-
end
575-
576-
def configured_assets_bundler
577-
config = parsed_shakapacker_config
578-
return nil unless config.is_a?(Hash)
579-
580-
rails_env = ENV["RAILS_ENV"] || ENV["RACK_ENV"] || "development"
581-
bundler_from_shakapacker_section(config, rails_env) || bundler_from_shakapacker_section(config, "default")
582-
rescue StandardError, ScriptError
583-
nil
584-
end
585-
558+
# Memoized per Doctor instance (a fresh Doctor runs per `doctor` invocation),
559+
# so a single diagnosis reads and parses config/shakapacker.yml at most once
560+
# even though several checks consult it. The base implementation lives in
561+
# ShakapackerConfigHelpers. ServerManager intentionally does not memoize its
562+
# copy because its helpers live on `class << self` and would leak across specs.
586563
def parsed_shakapacker_config
587-
config_path = shakapacker_config_path
588-
return nil unless File.exist?(config_path)
589-
590-
parse_shakapacker_config(File.read(config_path))
591-
rescue StandardError, ScriptError
592-
nil
593-
end
594-
595-
def bundler_from_shakapacker_section(config, section_name)
596-
section = config[section_name] || config[section_name.to_sym]
597-
return nil unless section.is_a?(Hash)
564+
return @parsed_shakapacker_config if defined?(@parsed_shakapacker_config)
598565

599-
normalize_assets_bundler(section["assets_bundler"] || section[:assets_bundler])
566+
@parsed_shakapacker_config = super
600567
end
601568

602-
def normalize_assets_bundler(value)
603-
normalized = value.to_s.strip.downcase
604-
ReactOnRails::SystemChecker::SUPPORTED_ASSETS_BUNDLERS.include?(normalized) ? normalized : nil
605-
end
606-
607-
def development_hmr_enabled?
608-
dev_server = development_dev_server_config
609-
return hmr_config_value?(dev_server["hmr"]) if dev_server.key?("hmr")
610-
611-
return false if truthy_config_value?(dev_server["live_reload"])
612-
613-
# Default to HMR when neither hmr nor live_reload is configured, preserving historical behavior.
614-
true
615-
end
616-
617-
def hmr_config_value?(value)
618-
value.to_s.strip.casecmp?("only") || truthy_config_value?(value)
619-
end
620-
621-
def development_dev_server_config
622-
config = parsed_shakapacker_config
623-
return {} unless config.is_a?(Hash)
624-
625-
development_config = shakapacker_section(config, "default").merge(shakapacker_section(config, "development"))
626-
dev_server_config_for(development_config)
627-
end
628-
629-
def shakapacker_section(config, section_name)
630-
section = config[section_name] || config[section_name.to_sym]
631-
section.is_a?(Hash) ? section : {}
632-
end
633-
634-
def dev_server_config_for(section)
635-
dev_server = section["dev_server"] || section[:dev_server]
636-
return {} unless dev_server.is_a?(Hash)
637-
638-
dev_server.transform_keys(&:to_s)
639-
end
640-
641-
def truthy_config_value?(value)
642-
value == true || value.to_s == "true"
643-
end
644-
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
645-
646569
def print_test_workflow_next_steps
647570
case @test_output_path_strategy
648571
when :shared
@@ -2069,20 +1992,6 @@ def uses_react_on_rails_test_helper?
20691992
# than the Rails root. Without this, a relative SHAKAPACKER_CONFIG would fail to resolve and
20701993
# dev-server mode detection would silently fall back to HMR labels. Falls back to Dir.pwd when
20711994
# Rails isn't booted (e.g. in unit specs).
2072-
def shakapacker_config_path
2073-
env_config_path = ENV.fetch("SHAKAPACKER_CONFIG", nil)
2074-
base = shakapacker_config_base_dir
2075-
return File.expand_path(DEFAULT_SHAKAPACKER_CONFIG_PATH, base) if env_config_path.to_s.empty?
2076-
2077-
File.expand_path(env_config_path, base)
2078-
end
2079-
2080-
def shakapacker_config_base_dir
2081-
return Rails.root.to_s if defined?(Rails) && Rails.respond_to?(:root) && Rails.root
2082-
2083-
Dir.pwd
2084-
end
2085-
20861995
def default_dev_server_mode
20871996
@default_dev_server_mode ||= Dev::ServerMode.detect(shakapacker_config_path)
20881997
end

0 commit comments

Comments
 (0)