-
Notifications
You must be signed in to change notification settings - Fork 17
chore(ci): test on node@22 and node@24 #247
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
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughUpdated the GitHub Actions workflow to introduce a root-level NODE_VERSION env var, expand the Node.js version matrix to include 22.x and 24.x, and standardize setup-node steps to use the env-driven version across build, test, and publish jobs. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
=======================================
Coverage 88.16% 88.16%
=======================================
Files 23 23
Lines 1217 1217
Branches 216 197 -19
=======================================
Hits 1073 1073
Misses 86 86
Partials 58 58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/main.yaml
(3 hunks)
🔇 Additional comments (2)
.github/workflows/main.yaml (2)
93-93
: Publishing on 24.x: make the source of the version explicit after removing the global env.If you remove the root-level env, this reference will break. Either hardcode the desired publish version or set a job-level env for publish.
Apply one of the following:
Option A — hardcode 24.x here:
- node-version: ${{ env.NODE_VERSION }} + node-version: '24.x'Option B — set job-level env and keep this line:
# inside `publish:` job env: NODE_VERSION: '24.x'Note: Consider publishing with the active LTS (22.x) unless you require Node 24 features. Verify your release pipeline (npm, node-gyp, optional native deps) is compatible with Node 24 before switching.
21-21
: CI Matrix & Engine Check for Node 16We’ve verified the
engines.node
fields in allpackage.json
files:
- repo root (
package.json
): ≥16.15.0example1/package.json
: ≥16.13.0opentelemetry/package.json
: ≥16.13.0No
.nvmrc
files were found and no other workflows pin Node 16. Since your packages still support Node 16, it’s safe to keep16.x
in the CI matrix. If you’d rather drop Node 16 to reduce EOL-related flakiness, please:
- Remove
16.x
from the matrix in.github/workflows/main.yaml
- Update all
engines.node
entries in yourpackage.json
files to>=18.x
d20f869
to
461c2b0
Compare
Audit failure is fixed by #248 |
@@ -48,15 +51,12 @@ jobs: | |||
- name: Set up node | |||
uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0 | |||
with: | |||
node-version: 20 | |||
node-version: ${{ env.NODE_VERSION }} |
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.
[question] Shouldn't this be ${{ matrix.node-version }}
? And I believe we shouldn't have any env with a node version at the top otherwise it will override all the builds. See comment from rabbitai https://github.com/openfga/js-sdk/pull/247/files#r2269791797
Description
Test on recent node versions, 22 (LTS) and 24 (current, upcoming LTS), change base build to node@24.
See: https://nodejs.org/en/about/previous-releases#release-schedule
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
main
Summary by CodeRabbit