-
-
Notifications
You must be signed in to change notification settings - Fork 2
chore: support node 20+ runtime #16
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
base: main
Are you sure you want to change the base?
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Version Compatibility Updates README.md, package.json, tsup.config.ts |
Updated Node.js minimum version from v24.11.1 to >=20. Modified engines.node field, adjusted tsup build target from 'node24' to 'node20', and added prerequisite note documenting CI coverage for versions 20.x, 22.x, and 24.x. |
Estimated code review effort
🎯 1 (Trivial) | ⏱️ ~3 minutes
Poem
🐰 Hop hop, the versions now align,
From twenty onwards, all will shine,
Node support grows wide and free,
Twenty, twenty-two, and twenty-four—happy spree! 🌱
Pre-merge checks and finishing touches
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check | ❓ Inconclusive | The PR partially addresses issue #15's objective. While it updates Node.js requirements from v24 to allow v22.19, the reviewer's feedback requested setting engines to '>=20' and tsup target to 'node20', but the PR only updated to '>=20' in package.json and 'node20' in tsup.config.ts without the suggested broader compatibility. | Verify that the implemented changes (engines: '>=20' and tsup target: 'node20') fully satisfy the reviewer's suggestion and the original issue's intent to support Node v22.19 and document broader compatibility. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Out of Scope Changes check | ✅ Passed | All changes (README.md Node prerequisite note, package.json engines field, tsup.config.ts build target) are directly related to the PR objective of updating Node.js version requirements and are within scope. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Title check | ✅ Passed | The PR title states 'chore: support node 20+ runtime' which accurately summarizes the main change: updating Node.js engine requirements from 24.11.1 to >=20. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
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 (1)
package.json (1)
52-52: Consider aligning @types/node version with minimum Node.js requirement.While @types/node version 24.10.1 doesn't prevent running on Node.js 22.x, having type definitions for Node.js 24 when the minimum runtime is 22.19.0 could lead to accidentally using Node 24+ features that would fail at runtime.
Consider downgrading to @types/node in the 22.x range to match the minimum supported runtime version and prevent inadvertent use of unavailable APIs.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.mise.toml(1 hunks)README.md(1 hunks)package.json(1 hunks)tsup.config.ts(1 hunks)
🔇 Additional comments (2)
README.md (1)
52-53: Update README documentation to reflect actual CI test matrix.The CI configuration tests on Node.js 20.x, 22.x, and 24.x, but the README (lines 52-53) only mentions testing on 22.x and 24.x. Additionally, if Node.js 22.19.0 is the minimum required version, testing on 20.x contradicts this requirement. Clarify whether:
- The minimum requirement should be updated to include Node.js 20.x support, or
- The CI matrix should be reduced to only 22.x and 24.x, and the README should be updated accordingly.
package.json (1)
14-14: Good change from exact to range constraint.The change from an exact version to a minimum version range is a best practice, allowing flexibility for users while enforcing the minimum requirement. The codebase is compatible with Node.js 22.19.0—it uses only standard Node.js APIs available since Node 12+ (such as
createRequirefromnode:module, standardProxy, and process APIs), with no Node 24+ specific features. The build target intsup.config.tsis already set tonode22, confirming this change aligns with the actual project configuration.
|
Thanks for opening this. In the CI setup, I even test on Can you update your PR accordingly? |
i reverted the mise change and adjusted to >=20 elsewhere. let me know if i misinterpreted your feedback or can otherwise adjust this PR. thanks - really appreciate the fast response/openness. |
Reduces node/engine version to
v22.19+v20+Fixes #15 (opened by me)
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.