-
-
Notifications
You must be signed in to change notification settings - Fork 638
Update preinstall script to enable use as a Git dependency #1873
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
Conversation
WalkthroughUpdates docs to document Git dependency installs and expand external-app testing guidance; moves react_on_rails_pro preinstall logic from an inline package.json script into a dedicated Node script and adds it to package files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant Installer as npm/yarn
participant pkg as package.json (preinstall)
participant script as preinstall.js
participant cmds as link-source & yalc
Developer->>Installer: run install
Installer->>pkg: invoke preinstall
pkg->>script: node ./script/preinstall.js
rect rgb(246, 250, 240)
note right of script: guard — detect if inside node_modules
script->>script: check path contains "node_modules"?
alt inside node_modules
script-->>pkg: log skip, exit 0
else
script->>cmds: run "yarn run link-source"
cmds-->>script: success / error
script->>cmds: run "yalc add --link react-on-rails"
cmds-->>script: success / error
alt any error
script-->>pkg: log error, exit 0
else
script-->>pkg: complete, exit 0
end
end
end
pkg-->>Installer: preinstall finished (0)
Installer-->>Developer: install finished
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
Code Review: PR #1873 - Update preinstall script to enable use as a Git dependencySummaryThis PR refactors the preinstall script from inline shell commands to a dedicated Node.js script, enabling the package to work properly when installed as a Git dependency. ✅ Strengths1. Code Quality
2. Implementation
🔍 Issues & RecommendationsCritical: Inconsistent Error HandlingLocation: react_on_rails_pro/script/preinstall.js:14-18 The runCommand function throws errors on failure, but the catch block at line 26 catches them and exits with code 0. This creates inconsistent behavior where exceptions are used for control flow. Recommendation: Since these commands are optional, consider removing the throws from runCommand and returning a boolean status instead. This makes the intent clearer and avoids using exceptions for control flow. Minor: Edge Case - Missing __dirnameLocation: preinstall.js:6 The fallback to process.cwd() is good defensive coding, but __dirname should always be defined in CommonJS scripts. This fallback might mask a real problem if __dirname is ever undefined. Recommendation: Consider adding a comment explaining when this fallback would be needed, or remove it if it's unnecessary. Minor: Error Object LoggingLocation: preinstall.js:29 Logging the entire error object may produce unclear output. Consider logging just the message for better readability. 🔒 Security Considerations✅ No security concerns identified
⚡ Performance Considerations✅ Performance is appropriate
🧪 Test CoverageWhile install scripts are notoriously difficult to test, consider:
📝 Documentation✅ PR description mentions documentation updates
🎯 Final RecommendationApprove with minor suggestions The PR successfully solves the Git dependency installation issue with a clean, cross-platform solution. The main concerns are around code clarity and error handling patterns rather than functional issues. Before merging:
Alignment with CLAUDE.md
Great work on improving the developer experience for Git dependency usage! 🚀 |
Code Review - PR #1873SummaryThis PR refactors the preinstall script to enable the react_on_rails_pro package to work as a Git dependency by moving the logic from a shell command string to a dedicated Node.js script. Positive Aspects
Code Quality Issues
The new file is missing a trailing newline character, which will cause CI to fail per the CLAUDE.md requirements. Fix: Add a newline after the final closing brace on line 31.
The check for node_modules could incorrectly match if someone has node_modules anywhere in their directory path. Recommendation: Use a more robust check that looks for node_modules as a directory boundary.
The fallback to process.cwd() is unnecessary since __dirname is always defined in regular Node.js scripts.
The generic error catch makes debugging harder. Consider adding more specific error information including the error message and code. Testing ConcernsIssue: The PR checklist shows test coverage is not complete. Recommendation: Consider adding tests for:
Security ConsiderationsNo security concerns identified. The script:
Performance ConsiderationsPerformance impact is minimal:
Additional Recommendations
Must Fix Before Merge
Overall AssessmentThis is a well-intentioned change that solves a real problem (Git dependency support). The code structure is clean and the approach is sound. However, there are critical formatting issues that will cause CI to fail, and some edge cases in the detection logic that should be addressed. Recommendation: Request changes to fix the critical formatting issue and improve the path detection logic before merging. Review generated by Claude Code following repository conventions from CLAUDE.md |
1b62ceb to
8d54e33
Compare
Code Review: PR #1873This PR successfully enables Git dependency support by refactoring the preinstall script. Overall assessment: Approve with minor suggestions Strengths
Issues & RecommendationsMinor: Node Modules Detection Edge CaseLocation: preinstall.js:7 The check could produce false positives if 'node_modules' appears anywhere in the directory path. Consider using a more robust check that verifies node_modules is a direct parent directory. Severity: Low Minor: Redundant FallbackLocation: preinstall.js:6 The fallback to process.cwd() is unnecessary since __dirname is always defined in CommonJS modules. Severity: Very Low Code Style: Exception Control FlowLocation: preinstall.js:14-18, 26-31 The runCommand function throws errors which are then caught and ignored. Consider refactoring to return status objects instead. Severity: Low (stylistic preference) Security & Performance
CLAUDE.md ComplianceBefore merging, verify:
Impact Assessment
Great work on improving the Git dependency support! Review by Claude Code following CLAUDE.md conventions |
Pull Request Review: Update preinstall script to enable use as a Git dependencySummaryThis PR successfully enables React on Rails packages to be installed as Git dependencies, which is valuable for development and testing. The changes are well-documented and the implementation is thoughtful. Code Quality & Best PracticesStrengths ✅
Areas for Improvement 🔍
Potential Bugs or IssuesMedium Priority
Performance Considerations
Security ConcernsLow Risk ✅
Recommendations
Test CoverageConcerns
|
b21f860 to
6a040ee
Compare
|
A better long-term solution is to make |
6a040ee to
57a325f
Compare
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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CONTRIBUTING.md (1)
109-117: Add language specifier to shell code block.This fenced code block is also missing a language identifier.
-``` +```sh cd <React on Rails root>/packages/react-on-rails # Will send the updates to other folders - MUST DO THIS AFTER ANY CHANGES yalc push cd <your project root> # Will update from yalc yarn -``` +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(2 hunks)CONTRIBUTING.md(2 hunks)react_on_rails_pro/package.json(1 hunks)react_on_rails_pro/script/preinstall.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- react_on_rails_pro/package.json
- react_on_rails_pro/script/preinstall.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
CONTRIBUTING.md
🪛 LanguageTool
CONTRIBUTING.md
[style] ~95-~95: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...led files yarn run build-watch ``` You need to do this once to make sure your app depe...
(REP_NEED_TO_VB)
[style] ~106-~106: Consider shortening or rephrasing this to strengthen your wording.
Context: ...eact-on-rails ``` The workflow is: 1. Make changes to the node package. 2. CRITICAL: Run ...
(MAKE_CHANGES)
[grammar] ~137-~137: Use a hyphen to join words.
Context: ...ing an update to the node package client side. Do the same for function `serverRe...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CONTRIBUTING.md
97-97: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
165-165: Bare URL used
(MD034, no-bare-urls)
173-173: Bare URL used
(MD034, no-bare-urls)
181-181: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22)
- GitHub Check: markdown-link-check
- GitHub Check: build-dummy-app-webpack-test-bundles (3.2, 20)
- GitHub Check: build
- GitHub Check: claude-review
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: lint-js-and-ruby
🔇 Additional comments (1)
CONTRIBUTING.md (1)
52-134: Approve expanded testing documentation for Git dependencies and local workflows.The restructured documentation comprehensively covers multiple testing approaches (local dependencies, Git dependencies, tarballs) with clear Ruby and JS-specific instructions. The port number updates (5000 → 3000), yalc workflow clarity, and practical examples like the NPM changes walkthrough (lines 135–139) are well-presented and should help contributors effectively test changes locally or via Git dependencies.
57a325f to
4d6458f
Compare
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CONTRIBUTING.md (2)
95-95: Optional style improvements flagged by static analysis.Two minor suggestions for consideration:
Line 95: The phrasing "You need to do this once" appears to have similar structure to nearby sentences (line 81). Consider varying the sentence structure for readability: "This step is only needed once to ensure your app depends on our package."
Line 106: The instruction "Make changes to the node package" could be more concise: "Change the node package" or "Modify the node package."
These are stylistic suggestions and not correctness issues.
Also applies to: 106-106
181-181: Minor readability consideration for the npm link (line 181).The current markdown format
[Explicitly doesn't want to support it.](url)makes the entire sentence the link text. For consistency with the other links in this section and improved readability, consider restructuring it:-[Explicitly doesn't want to support it.](https://github.com/npm/cli/issues/6253) +Explicitly doesn't want to support it: [see issue](https://github.com/npm/cli/issues/6253)This maintains the link while using more conventional markdown formatting and making the link text more descriptive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(2 hunks)CONTRIBUTING.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx,css,scss,json,yml,yaml,md}
📄 CodeRabbit inference engine (CLAUDE.md)
Prettier is the sole authority for formatting all non-Ruby files; never manually format them
Files:
CONTRIBUTING.md
🪛 LanguageTool
CONTRIBUTING.md
[style] ~95-~95: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...led files yarn run build-watch ``` You need to do this once to make sure your app depe...
(REP_NEED_TO_VB)
[style] ~106-~106: Consider shortening or rephrasing this to strengthen your wording.
Context: ...eact-on-rails ``` The workflow is: 1. Make changes to the node package. 2. CRITICAL: Run ...
(MAKE_CHANGES)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: claude-review
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: lint-js-and-ruby
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: markdown-link-check
🔇 Additional comments (1)
CONTRIBUTING.md (1)
52-182: Excellent work addressing all past review feedback.All critical issues from previous reviews have been resolved:
- Bare URLs are now properly wrapped in markdown links (lines 165, 173, 181)
- Compound adjective hyphenation corrected: "client-side" (line 137)
- Code blocks include language specifiers throughout
The reorganization substantially improves the documentation by clearly separating testing approaches (local, Git dependencies, tarball) and providing comprehensive examples for each. The new section structure makes it easier for contributors to find relevant guidance.
Summary
Documented use as a Git dependency and fixed an issue with the Pro Node renderer there.
Pull Request checklist
This change is
Summary by CodeRabbit
Bug Fixes
Documentation