Enable use as a Git dependency #1664
Conversation
WalkthroughThis pull request introduces several enhancements to the React on Rails project, focusing on version checking, package management, and server-side rendering capabilities. The changes include updating the version checking logic, adding support for Git dependencies, introducing streaming server rendering with React 18's Changes
Possibly Related PRs
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (9)lib/react_on_rails/version_checker.rb (5)
The addition of proper memoization using
The improved version checking now correctly handles all npm version expressions, with clear documentation reference. This change supports the PR's objective of enabling Git dependencies.
The renamed method with improved logic now correctly handles npm scoped packages (e.g., @scope/package) while still detecting local paths and URLs. This addresses the issue raised in previous reviews.
The error message is now more specific about version requirements, which will help developers understand and fix version-related issues faster.
The method call has been updated to match the renamed method, maintaining code consistency. spec/react_on_rails/version_checker_spec.rb (4)
The test doubles have been properly updated to reflect the renamed method, maintaining test consistency with the implementation. Also applies to: 119-124
Good addition of test coverage for version range expressions, ensuring the enhanced semver wildcard detection works correctly.
All test cases have been consistently updated to use the new method name, maintaining code clarity. Also applies to: 202-203, 218-219, 234-235, 250-252
Excellent addition of test coverage for Git URL handling, which directly supports the PR's objective of enabling Git dependencies. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
alexeyr-ci
force-pushed
the
alexeyr/prepack-script
branch
from
58df136 to
722b351
Compare
alexeyr-ci
changed the title
alexeyr-ci
force-pushed
the
alexeyr/prepack-script
branch
2 times, most recently
from
ef8be23 to
5b811d7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package-scripts.yml (1)
17-29: Documentation needed for Git dependency usage.Since this change enables using the package as a Git dependency, please update the documentation to include:
- Instructions for installing the package as a Git dependency
- Any prerequisites or considerations when using Git dependencies
- Troubleshooting steps for common build issues
Would you like me to help draft the documentation updates?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)package-scripts.yml(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- CHANGELOG.md
🔇 Additional comments (1)
package-scripts.yml (1)
17-29: The prepack script implementation looks good but could be improved.
The script successfully addresses the Git dependency issue by ensuring the package is built during npm pack or npm publish. However, there are some potential improvements:
Consider these enhancements:
build:
prepack:
description: Build the project in the prepack script.
# 1. If the project is already built, do nothing;
# 2. Build the project but ignore TypeScript errors from missing devDependencies;
# 3. Check if the project is built now;
# 4. If it failed, print an error message (still follow https://docs.npmjs.com/cli/v8/using-npm/scripts#best-practices).
script: >
[ -f node_package/lib/ReactOnRails.js ] ||
- (nps build >/dev/null 2>&1 || true) &&
+ (nps build >/dev/null 2>build-error.log || (cat build-error.log && false)) &&
[ -f node_package/lib/ReactOnRails.js ] ||
- { echo 'Building react-on-rails seems to have failed!'; }
+ { echo 'Building react-on-rails failed! Please check build-error.log for details or run `nps build` manually.'; exit 1; }This change:
- Captures build errors in a log file for debugging
- Returns a non-zero exit code on failure
- Provides more helpful error message with debugging instructions
Let's verify the build process works correctly:
alexeyr-ci
force-pushed
the
alexeyr/prepack-script
branch
from
5b811d7 to
212e25b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
26-26: Verify the changelog entry matches the PR objectiveThe changelog entry "Enable use as a
git:dependency" aligns with the PR objective but could be more descriptive about the actual changes made to enable this functionality. Consider expanding the entry to mention that it fixes the package build issue after downloading.Suggested improvement:
- Enable use as a `git:` dependency. [PR 1664](https://github.com/shakacode/react_on_rails/pull/1664) by [alexeyr-ci](https://github.com/alexeyr-ci). + Enable use as a `git:` dependency by ensuring package is built after downloading. [PR 1664](https://github.com/shakacode/react_on_rails/pull/1664) by [alexeyr-ci](https://github.com/alexeyr-ci).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)package-scripts.yml(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- package-scripts.yml
alexeyr-ci
changed the title
alexeyr-ci
force-pushed
the
alexeyr/prepack-script
branch
from
9b68f41 to
eac7ea0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package-scripts.yml (1)
17-31: LGTM! Robust implementation for Git dependency support.The implementation properly handles the build process when used as a Git dependency by:
- Checking if the project is already built
- Attempting to build if necessary
- Verifying the build succeeded
- Providing a clear error message on failure
Consider making the error message more specific:
- { echo 'Building react-on-rails seems to have failed!'; } + { echo 'Building react-on-rails failed! Please ensure all dependencies are installed.'; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.npmignore(0 hunks)CHANGELOG.md(1 hunks)package-scripts.yml(1 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- .npmignore
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (1)
CHANGELOG.md (1)
26-26: LGTM! Well-documented changelog entry.
The changelog entry is properly formatted, placed in the correct section, and accurately describes the fix with appropriate PR reference and author credit.
alexeyr-ci
force-pushed
the
alexeyr/prepack-script
branch
from
eac7ea0 to
4a535ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/react_on_rails/version_checker.rb (1)
23-23: Consider clarifying log behavior for local or Git dependencies
When the code detects a local path or URL (for example, a Git URL), it cleanly bypasses the version comparison. This is logically correct to avoid false mismatch warnings, but you may want to log a message indicating that version checks are skipped for local/Git dependencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
lib/react_on_rails/version_checker.rb(2 hunks)spec/react_on_rails/fixtures/git_package.json(1 hunks)spec/react_on_rails/fixtures/semver_range_package.json(1 hunks)spec/react_on_rails/version_checker_spec.rb(8 hunks)
✅ Files skipped from review due to trivial changes (2)
- spec/react_on_rails/fixtures/git_package.json
- spec/react_on_rails/fixtures/semver_range_package.json
🔇 Additional comments (12)
lib/react_on_rails/version_checker.rb (3)
112-112: Skipping version parse for local paths or URLs
Returning early if the package is local or Git-based ensures that you do not incorrectly parse a path as a semantic version. This looks appropriate given the intended logic.
116-116: Improved error message clarity
Raising an error with a specific message for non-exact versions is a good practice. This helps developers quickly identify the root cause of version incompatibilities.
98-101: Verify semver wildcard detection with pre-release identifiers
Because the regex checks for a dash, pre-release versions like "1.2.3-beta" will match and return true, indicating a wildcard. Consider handling standard semver pre-release tags (e.g., beta, rc) differently if you intend them to be treated as exact versions.
spec/react_on_rails/version_checker_spec.rb (9)
95-95: Test coverage for local path scenario
Including a test that sets a relative path and asserts no log is produced is valuable for ensuring local paths do not trigger version mismatch warnings.
119-124: Renaming parameter to match updated method name
Using “local_path_or_url” in the double ensures consistency with the refactored “local_path_or_url?” method. This helps maintain clarity between the production code and tests.
171-176: Good coverage for semver range detection
Verifying that version ranges (e.g., '>=1.2.3 <2.0.0') trigger the wildcard path ensures the updated checks align with npm dependencies' many version specifications.
186-187: Ensuring exact versions are not flagged as local
This test clarifies that “0.0.2” remains a standard semver version, confirming no local path detection occurs in such cases.
202-203: Confirming caret version is not mistaken for a local path
Testing “^14.0.0.beta-2” ensures the slash-based detection is not triggered for caret or pre-release semver references.
218-219: Relative path detection
Verifies that “../../..” is recognized correctly as a local path, preventing version parsing that might lead to errors.
234-235: Absolute file URL detection
Ensuring “file:///...” is recognized as a local path or URL supports Git-style or local protocol usage for advanced installation scenarios.
250-252: Yalc path scenario
Checks that “file:.yalc/react-on-rails” is properly flagged as a local path, vital for workflows using yalc.
259-267: Git URL test
Validating that “git://github.com/shakacode/react-on-rails.git” is recognized as local_path_or_url? ensures the logic supports Git-based installs as required by the PR’s objective.
alexeyr-ci
force-pushed
the
alexeyr/prepack-script
branch
from
8d2f7a6 to
9d359d8
Compare
Summary
When I tried to use a Git branch of the Node Renderer in Popmenu, it failed because the package isn't built after downloading. This PR fixes the issue. It should work for all package managers consuming this package:
prepareandprepack(and I think prepack is the more recommended) for Git dependencies;prepareprepackPull Request checklist
[ ] Add/update test to cover these changes[ ] Update documentationThis change is
Summary by CodeRabbit
New Features
git:dependency.Bug Fixes
Documentation
CHANGELOG.mdwith recent changes and enhancements.Chores