Skip to content

Fix duplicate messages in install generator #1788

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+35 −32
Open

Fix duplicate messages in install generator #1788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d31e517
Fix duplicate messages in install generator
ihabadham Sep 17, 2025
2b4e5ea
Fix RuboCop offense - move install_js_dependencies and other deps met…
ihabadham Sep 17, 2025
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
63 changes: 32 additions & 31 deletions lib/generators/react_on_rails/base_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,35 +100,7 @@ def add_js_dependencies
add_react_dependencies
add_css_dependencies
add_dev_dependencies
end

def install_js_dependencies
# Detect which package manager to use
success = if File.exist?(File.join(destination_root, "yarn.lock"))
run "yarn install"
elsif File.exist?(File.join(destination_root, "pnpm-lock.yaml"))
run "pnpm install"
elsif File.exist?(File.join(destination_root, "package-lock.json")) ||
File.exist?(File.join(destination_root, "package.json"))
# Use npm for package-lock.json or as default fallback
run "npm install"
else
true # No package manager detected, skip
end

unless success
GeneratorMessages.add_warning(<<~MSG.strip)
⚠️ JavaScript dependencies installation failed.

This could be due to network issues or missing package manager.
You can install dependencies manually later by running:
• npm install (if using npm)
• yarn install (if using yarn)
• pnpm install (if using pnpm)
MSG
end

success
install_js_dependencies
end
Comment on lines +103 to 104
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Duplicate installs still possible via fallbacks; gate bulk install and track fallbacks.

If add_npm_dependencies falls back to direct npm install inside any add_*_dependencies method, the final call to install_js_dependencies will run a second full install, reprinting output and wasting time. Track whether we used fallbacks vs. queued package.json edits, and skip the bulk install when appropriate.

Apply this diff to make the behavior single‑install:

@@
       def add_react_on_rails_package
@@
-        puts "Installing React on Rails package..."
-        return if add_npm_dependencies(react_on_rails_pkg)
+        puts "Installing React on Rails package..."
+        if add_npm_dependencies(react_on_rails_pkg)
+          @used_add_npm_dependencies = true
+          return
+        end
         puts "Using direct npm commands as fallback"
-        success = run "npm install #{react_on_rails_pkg.join(' ')}"
+        success = run "npm install #{react_on_rails_pkg.join(' ')}"
+        @did_any_direct_install = true if success
         handle_npm_failure("react-on-rails package", react_on_rails_pkg) unless success
       end
@@
       def add_react_dependencies
@@
-        return if add_npm_dependencies(react_deps)
+        if add_npm_dependencies(react_deps)
+          @used_add_npm_dependencies = true
+          return
+        end
@@
-        success = run "npm install #{react_deps.join(' ')}"
+        success = run "npm install #{react_deps.join(' ')}"
+        @did_any_direct_install = true if success
         handle_npm_failure("React dependencies", react_deps) unless success
       end
@@
       def add_css_dependencies
@@
-        return if add_npm_dependencies(css_deps)
+        if add_npm_dependencies(css_deps)
+          @used_add_npm_dependencies = true
+          return
+        end
@@
-        success = run "npm install #{css_deps.join(' ')}"
+        success = run "npm install #{css_deps.join(' ')}"
+        @did_any_direct_install = true if success
         handle_npm_failure("CSS dependencies", css_deps) unless success
       end
@@
       def add_dev_dependencies
@@
-        return if add_npm_dependencies(dev_deps, dev: true)
+        if add_npm_dependencies(dev_deps, dev: true)
+          @used_add_npm_dependencies = true
+          return
+        end
@@
-        success = run "npm install --save-dev #{dev_deps.join(' ')}"
+        success = run "npm install --save-dev #{dev_deps.join(' ')}"
+        @did_any_direct_install = true if success
         handle_npm_failure("development dependencies", dev_deps, dev: true) unless success
       end

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In lib/generators/react_on_rails/base_generator.rb around lines 103-104, prevent
a second full JS install by tracking whether any add_*_dependencies method
performed a direct fallback install and skipping the final
install_js_dependencies when so; add an instance boolean (e.g. @used_fallbacks =
false) initialized in the generator, set it to true inside add_npm_dependencies
(and any other fallback branches) when you call npm/yarn directly, and only call
install_js_dependencies at the end if @used_fallbacks is false (so queued
package.json edits trigger the bulk install, but direct fallback installs do not
cause a second install).


def update_gitignore_for_auto_registration
Expand Down Expand Up @@ -157,6 +129,8 @@ def append_to_spec_rails_helper
end
end

private

def add_react_on_rails_package
major_minor_patch_only = /\A\d+\.\d+\.\d+\z/

Expand Down Expand Up @@ -219,6 +193,35 @@ def add_dev_dependencies
handle_npm_failure("development dependencies", dev_deps, dev: true) unless success
end

def install_js_dependencies
# Detect which package manager to use
success = if File.exist?(File.join(destination_root, "yarn.lock"))
run "yarn install"
elsif File.exist?(File.join(destination_root, "pnpm-lock.yaml"))
run "pnpm install"
elsif File.exist?(File.join(destination_root, "package-lock.json")) ||
File.exist?(File.join(destination_root, "package.json"))
# Use npm for package-lock.json or as default fallback
run "npm install"
else
true # No package manager detected, skip
end

unless success
GeneratorMessages.add_warning(<<~MSG.strip)
⚠️ JavaScript dependencies installation failed.

This could be due to network issues or missing package manager.
You can install dependencies manually later by running:
• npm install (if using npm)
• yarn install (if using yarn)
• pnpm install (if using pnpm)
MSG
end

success
end

CONFIGURE_RSPEC_TO_COMPILE_ASSETS = <<-STR.strip_heredoc
RSpec.configure do |config|
# Ensure that if we are running js tests, we are using latest webpack assets
Expand All @@ -227,8 +230,6 @@ def add_dev_dependencies
end
STR

private

def handle_npm_failure(dependency_type, packages, dev: false)
install_command = dev ? "npm install --save-dev" : "npm install"
GeneratorMessages.add_warning(<<~MSG.strip)
Expand Down
4 changes: 3 additions & 1 deletion lib/generators/react_on_rails/install_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ def run_generators
if installation_prerequisites_met? || options.ignore_warnings?
invoke_generators
add_bin_scripts
add_post_install_message
# Only add the post install message if not using Redux
# Redux generator handles its own messages
add_post_install_message unless options.redux?
else
error = <<~MSG.strip
🚫 React on Rails generator prerequisites not met!
Expand Down
Loading