Skip to content

Commit 74b9038

Browse files
justin808claude
andcommitted
fix: Address code review issues in JS dependency manager
This commit addresses all code review feedback: 1. Improved instance variable initialization pattern - Changed from @added_dependencies_to_package_json ||= false - To: @added_dependencies_to_package_json = false unless defined?(@added_dependencies_to_package_json) - Added clear documentation explaining the initialization is done by the module 2. Made error messages more specific - All raise statements now include which packages failed - Added context that failure indicates shakapacker may not be properly installed - Error messages now help users understand what went wrong 3. Documented add_js_dependency method usage - Added YARD-style documentation - Clarified it's used for single package with version (react-on-rails@VERSION) - Explained add_js_dependencies_batch is for batch operations - Both methods now have clear @param and @return documentation 4. Fixed TypeScript dependency installation inconsistency - Removed manual instance variable setting - Now uses consistent pattern with shared module approach - Raises specific error instead of just warning if installation fails - Matches the error handling pattern of other dependency methods 5. Fixed RuboCop violations - Removed redundant begin block in install_js_dependencies - All files now pass bundle exec rubocop with zero offenses All tests pass successfully (6 examples, 0 failures). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
1 parent fd56259 commit 74b9038

File tree

2 files changed

+45
-38
lines changed

2 files changed

+45
-38
lines changed

lib/generators/react_on_rails/install_generator.rb

Lines changed: 6 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -374,26 +374,14 @@ def install_typescript_dependencies
374374
@babel/preset-typescript
375375
]
376376

377-
# Try using GeneratorHelper first (package manager agnostic)
378-
if add_npm_dependencies(typescript_packages, dev: true)
379-
@added_dependencies_to_package_json = true
380-
return
381-
end
382-
383-
# Fallback to npm if GeneratorHelper fails
384-
success = system("npm", "install", "--save-dev", *typescript_packages)
385-
if success
386-
@ran_direct_installs = true
387-
return
377+
# Use the shared dependency management approach via GeneratorHelper
378+
unless add_npm_dependencies(typescript_packages, dev: true)
379+
# This should not happen since package_json is always available via shakapacker
380+
raise "Failed to add TypeScript dependencies (#{typescript_packages.join(', ')}) via package_json gem. " \
381+
"This indicates shakapacker dependency may not be properly installed."
388382
end
389383

390-
warning = <<~MSG.strip
391-
⚠️ Failed to install TypeScript dependencies automatically.
392-
393-
Please run manually:
394-
npm install --save-dev #{typescript_packages.join(' ')}
395-
MSG
396-
GeneratorMessages.add_warning(warning)
384+
@added_dependencies_to_package_json = true
397385
end
398386

399387
def create_css_module_types

lib/generators/react_on_rails/js_dependency_manager.rb

Lines changed: 39 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,9 @@ module JsDependencyManager
5252
private
5353

5454
def setup_js_dependencies
55-
@added_dependencies_to_package_json ||= false
55+
# Initialize instance variable if not already defined by including class
56+
# This ensures safe operation when the module is first included
57+
@added_dependencies_to_package_json = false unless defined?(@added_dependencies_to_package_json)
5658
add_js_dependencies
5759
install_js_dependencies
5860
end
@@ -73,7 +75,8 @@ def add_react_on_rails_package
7375
@added_dependencies_to_package_json = true
7476
else
7577
# This should not happen since package_json is always available via shakapacker
76-
raise "Failed to add react-on-rails package via package_json gem"
78+
raise "Failed to add react-on-rails package via package_json gem. " \
79+
"This indicates shakapacker dependency may not be properly installed."
7780
end
7881
end
7982

@@ -84,7 +87,8 @@ def add_react_dependencies
8487
@added_dependencies_to_package_json = true
8588
else
8689
# This should not happen since package_json is always available via shakapacker
87-
raise "Failed to add React dependencies via package_json gem"
90+
raise "Failed to add React dependencies (#{REACT_DEPENDENCIES.join(', ')}) via package_json gem. " \
91+
"This indicates shakapacker dependency may not be properly installed."
8892
end
8993
end
9094

@@ -95,7 +99,8 @@ def add_css_dependencies
9599
@added_dependencies_to_package_json = true
96100
else
97101
# This should not happen since package_json is always available via shakapacker
98-
raise "Failed to add CSS dependencies via package_json gem"
102+
raise "Failed to add CSS dependencies (#{CSS_DEPENDENCIES.join(', ')}) via package_json gem. " \
103+
"This indicates shakapacker dependency may not be properly installed."
99104
end
100105
end
101106

@@ -106,11 +111,20 @@ def add_dev_dependencies
106111
@added_dependencies_to_package_json = true
107112
else
108113
# This should not happen since package_json is always available via shakapacker
109-
raise "Failed to add development dependencies via package_json gem"
114+
raise "Failed to add development dependencies (#{DEV_DEPENDENCIES.join(', ')}) via package_json gem. " \
115+
"This indicates shakapacker dependency may not be properly installed."
110116
end
111117
end
112118

113119
# Add a single dependency using package_json gem
120+
#
121+
# This method is used internally for adding the react-on-rails package
122+
# with version-specific handling (react-on-rails@VERSION).
123+
# For batch operations, use add_js_dependencies_batch instead.
124+
#
125+
# @param package [String] Package specifier (e.g., "[email protected]")
126+
# @param dev [Boolean] Whether to add as dev dependency
127+
# @return [Boolean] true if successful, false otherwise
114128
def add_js_dependency(package, dev: false)
115129
pj = package_json
116130
return false unless pj
@@ -131,28 +145,33 @@ def add_js_dependency(package, dev: false)
131145
end
132146

133147
# Add multiple dependencies at once using package_json gem
148+
#
149+
# This method delegates to GeneratorHelper's add_npm_dependencies for
150+
# better package manager abstraction and batch processing efficiency.
151+
#
152+
# @param packages [Array<String>] Package names to add
153+
# @param dev [Boolean] Whether to add as dev dependencies
154+
# @return [Boolean] true if successful, false otherwise
134155
def add_js_dependencies_batch(packages, dev: false)
135156
# Use the add_npm_dependencies helper from GeneratorHelper
136157
add_npm_dependencies(packages, dev: dev)
137158
end
138159

139160
def install_js_dependencies
140161
# Use package_json gem's install method (always available via shakapacker)
141-
begin
142-
package_json.manager.install
143-
true
144-
rescue StandardError => e
145-
GeneratorMessages.add_warning(<<~MSG.strip)
146-
⚠️ JavaScript dependencies installation failed: #{e.message}
147-
148-
This could be due to network issues or package manager problems.
149-
You can install dependencies manually later by running:
150-
• npm install (if using npm)
151-
• yarn install (if using yarn)
152-
• pnpm install (if using pnpm)
153-
MSG
154-
false
155-
end
162+
package_json.manager.install
163+
true
164+
rescue StandardError => e
165+
GeneratorMessages.add_warning(<<~MSG.strip)
166+
⚠️ JavaScript dependencies installation failed: #{e.message}
167+
168+
This could be due to network issues or package manager problems.
169+
You can install dependencies manually later by running:
170+
• npm install (if using npm)
171+
• yarn install (if using yarn)
172+
• pnpm install (if using pnpm)
173+
MSG
174+
false
156175
end
157176

158177
# No longer needed since package_json gem handles package manager detection

0 commit comments

Comments
 (0)