Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 3 additions & 5 deletions packages/create-react-on-rails-app/src/create-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -226,11 +226,9 @@ export function validateAppName(name: string): { success: boolean; error?: strin
export function createApp(appName: string, options: CliOptions): void {
const appPath = path.resolve(process.cwd(), appName);
const proRequested = options.pro || options.rsc;
let proModeLabel: string | null = null;
if (options.rsc) {
proModeLabel = '--rsc';
} else if (options.pro) {
proModeLabel = '--pro';
let proModeLabel: '--rsc' | '--pro' | null = null;
if (proRequested) {
proModeLabel = options.rsc ? '--rsc' : '--pro';
}
const baseSteps = 3; // rails new + add react_on_rails + run generator
const totalSteps = baseSteps + (proRequested ? 1 : 0);
Expand Down
76 changes: 63 additions & 13 deletions react_on_rails/lib/react_on_rails/config_path_resolver.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,18 @@

module ReactOnRails
module ConfigPathResolver
# Keep JS before TS to match generator defaults and to prefer the
# JavaScript config deterministically when both variants are present.
WEBPACK_DEFAULT_CONFIG_CANDIDATES = %w[
config/webpack/webpack.config.js
config/webpack/webpack.config.ts
].freeze
RSPACK_DEFAULT_CONFIG_CANDIDATES = %w[
config/rspack/rspack.config.js
config/rspack/rspack.config.ts
].freeze
ALL_DEFAULT_CONFIG_CANDIDATES = (WEBPACK_DEFAULT_CONFIG_CANDIDATES + RSPACK_DEFAULT_CONFIG_CANDIDATES).freeze

private

def resolved_package_json_path
Expand All @@ -12,39 +24,77 @@ def resolved_package_json_path
end

def resolved_webpack_config_path
webpack_config_candidates.find { |path| File.exist?(path) }
webpack_config_candidates.find { |path| File.file?(path) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

resolved_webpack_config_path is called from at least three places — bundler_config_file_exists?, detect_bundler_config_path, and (indirectly) explicit_shakapacker_bundler_config_path? — and each call rebuilds webpack_config_candidates and does N×File.file? syscalls. shakapacker_assets_bundler_config_path was carefully memoized to avoid re-requiring Shakapacker; resolved_webpack_config_path should get the same treatment for consistency and to avoid redundant filesystem probing in a single doctor run.

Suggested change
webpack_config_candidates.find { |path| File.file?(path) }
def resolved_webpack_config_path
return @resolved_webpack_config_path if instance_variable_defined?(:@resolved_webpack_config_path)
@resolved_webpack_config_path = webpack_config_candidates.find { |path| File.file?(path) }
end

end

def webpack_config_candidates
candidates = []
shakapacker_config_path = shakapacker_assets_bundler_config_path
candidates << shakapacker_config_path if shakapacker_config_path

shakapacker_config_dir = shakapacker_webpack_config_directory
shakapacker_config_dir = bundler_config_directory(shakapacker_config_path)
if shakapacker_config_dir
candidates.concat(%w[js ts cjs mjs].flat_map do |ext|
# Skip only the exact shakapacker path; still probe sibling
# standard-name configs (for example rspack when shakapacker points to
# webpack in the same directory).
[
File.join(shakapacker_config_dir, "webpack.config.#{ext}"),
File.join(shakapacker_config_dir, "rspack.config.#{ext}")
]
].reject { |path| path == shakapacker_config_path }
end)
end

candidates << "config/webpack/webpack.config.js"
candidates << "config/webpack/webpack.config.ts"
candidates << "config/rspack/rspack.config.js"
candidates << "config/rspack/rspack.config.ts"
# Default fallback candidates intentionally mirror generator defaults
# (`.js` / `.ts`), while `.cjs` / `.mjs` are probed only within resolved
# shakapacker config directories above.
candidates.concat(WEBPACK_DEFAULT_CONFIG_CANDIDATES)
candidates.concat(RSPACK_DEFAULT_CONFIG_CANDIDATES)
candidates.uniq
Comment on lines +51 to 53

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The comment says .uniq removes "accidental" duplicates, but when Shakapacker's assets_bundler_config_path happens to equal a default candidate (e.g. config/webpack/webpack.config.js), that path is intentionally inserted twice — first at line 33 as the explicit Shakapacker path, then again here via concat(WEBPACK_DEFAULT_CONFIG_CANDIDATES). The .uniq is therefore load-bearing correctness for this case, not just a safety net. Worth clarifying the comment so future readers don't treat .uniq as removable cleanup:

Suggested change
candidates.concat(WEBPACK_DEFAULT_CONFIG_CANDIDATES)
candidates.concat(RSPACK_DEFAULT_CONFIG_CANDIDATES)
candidates.uniq
# Default fallback candidates intentionally mirror generator defaults
# (`.js` / `.ts`), while `.cjs` / `.mjs` are probed only within resolved
# shakapacker config directories above.
# NOTE: When Shakapacker's assets_bundler_config_path is itself one of
# these defaults (e.g. config/webpack/webpack.config.js), it will appear
# in both the explicit-path slot above and here. The `.uniq` call below is
# load-bearing for that case, not just a cleanup step.
candidates.concat(WEBPACK_DEFAULT_CONFIG_CANDIDATES)
candidates.concat(RSPACK_DEFAULT_CONFIG_CANDIDATES)

end

def shakapacker_webpack_config_directory
def shakapacker_assets_bundler_config_path
# Use instance_variable_defined? instead of ||= so nil results are cached
# and we do not retry require/rescue work on each call.
if instance_variable_defined?(:@shakapacker_assets_bundler_config_path)
return @shakapacker_assets_bundler_config_path
end

require "shakapacker"
path = Shakapacker.config.assets_bundler_config_path.to_s
@shakapacker_assets_bundler_config_path = normalize_shakapacker_assets_bundler_config_path(
Shakapacker.config.assets_bundler_config_path.to_s
)
rescue LoadError, NameError
# Doctor/install checks should degrade gracefully when Shakapacker is
# missing; callers fall back to discovered default config candidates.
@shakapacker_assets_bundler_config_path = nil
rescue StandardError => e
message = "ReactOnRails could not read Shakapacker assets_bundler_config_path: #{e.class}: #{e.message}"
warn(message) unless Rails.logger

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The warn call here outputs to stderr when Rails.logger is nil. Since this is a debug-level diagnostic (Shakapacker config read failure), consider whether stderr noise is acceptable in environments where Rails logger hasn't been set up yet (e.g. during early boot). The logic is correct — warn fires only when logger is absent, logger.debug fires when present — but the severity mismatch (warn-to-stderr for a debug situation) may surprise operators.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good callout. I’m keeping this behavior as-is for now: when Rails.logger is unavailable, this fallback path uses warn intentionally so configuration-read failures are still visible during early boot/diagnostics. Once logger is available, it stays at debug level via Rails.logger.debug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The two sequential statements are correct — warn fires when logger is nil, &.debug fires when logger is present — but easy to misread as both always executing. A single if/else makes the mutual exclusion explicit:

Suggested change
warn(message) unless Rails.logger
if Rails.logger
Rails.logger.debug { message }
else
warn(message)
end

Rails.logger&.debug do
message
end
@shakapacker_assets_bundler_config_path = nil
end

def normalize_shakapacker_assets_bundler_config_path(path)
return nil if path.empty?

directory = File.dirname(path)
rails_root = Rails.root.to_s
directory.start_with?("#{rails_root}/") ? directory.sub("#{rails_root}/", "") : directory
rescue LoadError, StandardError
nil
return path if rails_root.empty? || rails_root == "/"

# NOTE: Prefix normalization assumes matching separators and does not
# normalize Windows-style `\` vs `/` path variants.
rails_root_prefix = "#{rails_root}/"
normalized_path = path.start_with?(rails_root_prefix) ? path.delete_prefix(rails_root_prefix) : path
normalized_path.empty? ? nil : normalized_path
end

def bundler_config_directory(config_path)
return nil unless config_path

directory = File.dirname(config_path)
directory == "." ? nil : directory
end
end
end
6 changes: 4 additions & 2 deletions react_on_rails/lib/react_on_rails/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ def self.configure
DEFAULT_GENERATED_ASSETS_DIR = File.join(%w[public webpack], Rails.env).freeze
DEFAULT_COMPONENT_REGISTRY_TIMEOUT = 5000
DEFAULT_SERVER_BUNDLE_OUTPUT_PATH = "ssr-generated"
DEFAULT_SERVER_RENDERER_POOL_SIZE = 1
DEFAULT_SERVER_RENDERER_TIMEOUT_SECONDS = 20
Comment thread
justin808 marked this conversation as resolved.

def self.configuration
@configuration ||= Configuration.new(
Expand All @@ -44,8 +46,8 @@ def self.configuration
raise_on_prerender_error: Rails.env.development?,
trace: Rails.env.development?,
development_mode: Rails.env.development?,
server_renderer_pool_size: 1,
server_renderer_timeout: 20,
server_renderer_pool_size: DEFAULT_SERVER_RENDERER_POOL_SIZE,
server_renderer_timeout: DEFAULT_SERVER_RENDERER_TIMEOUT_SECONDS,
skip_display_none: nil,
# skip_display_none is deprecated
webpack_generated_files: %w[manifest.json],
Expand Down
Loading
Loading