-
Notifications
You must be signed in to change notification settings - Fork 153
chore: upgrade node and pnpm to latest LTS version #1073
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: develop
Are you sure you want to change the base?
chore: upgrade node and pnpm to latest LTS version #1073
Conversation
Summary by CodeRabbit
Note: You may need to upgrade your local Node.js to v22 to develop or run the project. WalkthroughUpdated CI to use Node.js 22.x. package.json now specifies pnpm as the package manager, raises the Node engine to >=22, and updates Volta’s pinned Node to 22.19.0. pnpm-workspace.yaml adds onlyBuiltDependencies entries for selected packages. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
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.
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/continuous-integration.yml (3)
21-31: Align toolchain versions and enable caching.
- Use the same checkout action in both jobs and bump to the latest stable major.
- Install pnpm matching packageManager (10.10.0) instead of “latest”.
- Enable setup-node cache for pnpm to speed installs.
Apply:
- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 ... - - name: Install pnpm - uses: pnpm/action-setup@v2 - with: - version: latest + - name: Install pnpm + uses: pnpm/action-setup@v2 + with: + version: 10.10.0 ... - - name: Install Dependencies - run: pnpm install --frozen-lockfile + - name: Setup Node with cache + uses: actions/setup-node@v4 + with: + node-version: ${{ matrix.node-version }} + cache: 'pnpm' + - name: Install Dependencies + run: pnpm install --frozen-lockfileAnd in the build job:
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4 ... - - name: Install pnpm - uses: pnpm/action-setup@v2 - with: - version: latest + - name: Install pnpm + uses: pnpm/action-setup@v2 + with: + version: 10.10.0 + - name: Setup Node with cache + uses: actions/setup-node@v4 + with: + node-version: ${{ matrix.node-version }} + cache: 'pnpm'Also applies to: 42-54
35-37: Set a timeout for the build job.Lint has timeout-minutes: 10; add one to build to avoid hung CI.
Apply:
build: runs-on: ubuntu-latest + timeout-minutes: 20
55-57: Avoid double-running lint.The build job’s “pnpm test” triggers “pnpm:lint” per your test script, while a separate lint job already runs. Consider running tests only here to save CI time.
- - name: Test - run: pnpm test + - name: Test + run: pnpm run test:ember
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
.github/workflows/continuous-integration.yml(2 hunks)package.json(2 hunks)pnpm-workspace.yaml(1 hunks)
⏰ 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). (1)
- GitHub Check: build (22.x)
🔇 Additional comments (2)
pnpm-workspace.yaml (1)
1-6: Verify allowlist completeness for onlyBuiltDependencies
Your workspace is using lockfileVersion 9.0, which doesn’t includerequiresBuildmetadata, so we can’t auto-detect build‐script deps. After upgrading to pnpm ≥ 10.10.0, re-run withrequiresBuild⊢true detection to confirm nothing’s missing, or manually audit each dependency’s install/build scripts.package.json (1)
118-118: Engine bump to Node >=22 acknowledged; ensure downstream tooling supports it.Ember CLI 5.9 and related tooling generally support Node 22, but verify locally and in CI. If issues arise, consider pinning CI to 22.19.0 (Volta version).
| strategy: | ||
| matrix: | ||
| node-version: [18.x] | ||
| node-version: [22.x] |
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.
🧹 Nitpick (assertive)
Node 22.x matrix looks good. Consider pinning to your Volta version for reproducibility.
You pin Volta to 22.19.0 in package.json; CI currently uses 22.x. Pinning avoids drift.
Apply:
- node-version: [22.x]
+ node-version: [22.19.0]Also applies to: 40-40
🤖 Prompt for AI Agents
.github/workflows/continuous-integration.yml lines 18 and 40: the workflow uses
a floating node-version (22.x) which can drift from the pinned Volta/node
version in package.json; update the matrix entries to use the exact version
pinned in package.json (22.19.0) so CI uses the same Node release as Volta,
replacing 22.x with 22.19.0 at both locations.
| "repository": "", | ||
| "license": "MIT", | ||
| "author": "", | ||
| "packageManager": "pnpm@10.10.0", |
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.
🛠️ Refactor suggestion
Pin pnpm in CI to match packageManager.
You set packageManager to pnpm@10.10.0; CI installs “latest”. Align to 10.10.0 to avoid policy drift with onlyBuiltDependencies.
See workflow diffs to set pnpm/action-setup version: 10.10.0.
🤖 Prompt for AI Agents
In package.json around line 9, packageManager is set to "pnpm@10.10.0" but CI
currently installs the latest pnpm; update the CI workflow(s) that set up pnpm
(e.g., .github/workflows/*) to pin the pnpm setup action/version to 10.10.0 so
the runner uses pnpm@10.10.0, ensuring consistency with packageManager and
preventing policy drift with onlyBuiltDependencies.
| }, | ||
| "volta": { | ||
| "node": "18.19.1" | ||
| "node": "22.19.0" |
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.
🧹 Nitpick (assertive)
Volta pin looks good. Consider matching CI to 22.19.0.
Keeps parity between local and CI builds.
🤖 Prompt for AI Agents
In package.json at line 124 (package.json:124-124), Volta is pinned to "node":
"22.19.0" but CI workflows are not guaranteed to match; update your CI
configuration(s) (e.g., .github/workflows/*, .gitlab-ci.yml,
azure-pipelines.yml, or any Dockerfiles) to use Node 22.19.0 so local Volta
parity is preserved — specifically set actions/setup-node or the matrix
node-version value, update any nvm/engine or Docker base images to 22.19.0, and
run CI to confirm builds pass.
| onlyBuiltDependencies: | ||
| - '@fortawesome/fontawesome-common-types' | ||
| - '@fortawesome/free-solid-svg-icons' | ||
| - core-js | ||
| - pre-commit | ||
| - spawn-sync |
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.
🛠️ Refactor suggestion
Revisit entries: prefer adding actual native/build deps (e.g., esbuild, sharp, canvas) and drop no-op packages.
Font Awesome icon packs and core-js are pure JS and typically don’t need build scripts; “pre-commit” alters git hooks and is usually unnecessary in CI. Keep the list minimal and accurate to reduce surprise failures.
🤖 Prompt for AI Agents
In pnpm-workspace.yaml around lines 1 to 6, the onlyBuiltDependencies list
contains packages that are pure JS or irrelevant to native build steps (e.g.,
Font Awesome packs, core-js, pre-commit, spawn-sync); replace/remove these
entries and instead list true native/build-time dependencies your monorepo needs
(for example esbuild, sharp, canvas) and drop no-op or tooling-only packages
(like pre-commit) so the onlyBuiltDependencies accurately reflects packages that
require native builds; ensure the list is minimal and validated against packages
that actually run native install scripts.
Date: 09-09-2025
Developer Name: @MayankBansal12
Issue Ticket Number:-
Description:
Is Under Feature Flag
Database changes
Breaking changes (If your feature is breaking/missing something please mention pending tickets)
Is Development Tested?
Tested in staging?
Add relevant Screenshot below ( e.g test coverage etc. )
Description by Korbit AI
What change is being made?
Upgrade the Node.js version to 22.x and update pnpm to version 10.10.0 across configuration files.
Why are these changes being made?
This update ensures that the project aligns with the latest Long Term Support (LTS) versions of Node.js and pnpm, providing improved security, stability, and access to the latest features. The addition of the
pnpm-workspace.yamlfile indicates an optimization in package management workflow by specifying built dependencies, ensuring faster and leaner builds.