Conversation
`engines` requires `node` version `>=22.11.0` instead of `>=22`, while `22.11.0` is first long term support minor version of this major version. `devEngines` requires `node` version `^22.11.0 || ^24.11.0` instead of `22.x || 24.x`, while `22.11.0` and `24.11.0` are first long term support minor versions of those major versions. `devEngines` now requires `npm` version `^11.6.1` as it is first `npm` version delivered by `node` in version `24.11.0`. We require single major version of `npm` to generate `package-lock.json` of compatible structure. This would not be possible without requiring specific version of `npm`.
There was a problem hiding this comment.
Pull request overview
This PR tightens the Node.js and npm version requirements to align with Long Term Support (LTS) versions. The engines field now requires Node.js >=22.11.0 (the first LTS minor of v22), while devEngines restricts development to specific LTS versions (^22.11.0 || ^24.11.0) and introduces npm version constraint ^11.6.1 to ensure consistent package-lock.json structure.
Key Changes:
- Updated
engines.nodefrom>=22to>=22.11.0to require the first LTS minor version - Changed
devEngines.runtime.versionfrom22.x || 24.xto^22.11.0 || ^24.11.0for stricter LTS-only development - Added
devEngines.packageManager.versionrequirement of^11.6.1for npm
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| package.json | Updated engine requirements to specific LTS versions and added npm version constraint |
| package-lock.json | Synchronized engine requirements with package.json and added devEngines configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "node": ">=22.11.0" | ||
| }, | ||
| "devEngines": { | ||
| "runtime": { | ||
| "name": "node", | ||
| "version": "22.x || 24.x", | ||
| "version": "^22.11.0 || ^24.11.0", |
There was a problem hiding this comment.
There's an inconsistency between engines.node and devEngines.runtime.version. The engines field allows Node.js >=22.11.0 (including 23.x, 25.x, etc.), while devEngines restricts to only ^22.11.0 || ^24.11.0 (which excludes 23.x, 25.x). This means engines would pass but devEngines would fail with an error for versions like 23.x or 25.x.
Consider either:
- Aligning
devEnginesto matchenginesby using>=22.11.0 <25.0.0(if 24.x is the max supported), or - Tightening
enginesto match the actual supported versions if only 22.x and 24.x LTS lines are intended to be supported.
There was a problem hiding this comment.
We want to have strictly tight dev environment, but we do not want to tight end-users.
cbcd652 to
b1e6d73
Compare
.github/workflows/build.yml
Outdated
| NPM_VERSION: 11 | ||
|
|
||
| jobs: | ||
| prepare_env: |
There was a problem hiding this comment.
prepare_env is need as matrix does not allow to access env as steps does.
.github/workflows/build.yml
Outdated
| node-version: ${{ matrix.node }} | ||
| cache: npm | ||
| if: matrix.node == env.NODE_PREVIOUS_LTS_VERSION | ||
| continue-on-error: true |
There was a problem hiding this comment.
continue-on-error: true is needed as Node 22 is shipped with npm@10 and devEngines requires 11. This allow us to continue to Install npm@11 for Node.js step where up-to-date version is installed.
b1e6d73 to
6c5d827
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/build.yml
Outdated
| prepare_env: | ||
| name: Prepare environment | ||
| runs-on: ubuntu-24.04 | ||
| outputs: | ||
| NODE_ACTIVE_LTS_VERSION: ${{ env.NODE_ACTIVE_LTS_VERSION }} | ||
| NODE_PREVIOUS_LTS_VERSION: ${{ env.NODE_PREVIOUS_LTS_VERSION }} | ||
| NPM_VERSION: ${{ env.NPM_VERSION }} | ||
| steps: | ||
| - name: Compute outputs | ||
| run: | | ||
| echo "NODE_ACTIVE_LTS_VERSION=${{ env.NODE_ACTIVE_LTS_VERSION }}" >> $GITHUB_OUTPUT | ||
| echo "NODE_PREVIOUS_LTS_VERSION=${{ env.NODE_PREVIOUS_LTS_VERSION }}" >> $GITHUB_OUTPUT | ||
| echo "NPM_VERSION=${{ env.NPM_VERSION }}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
The prepare_env job appears to only pass through environment variables as outputs without any transformation. Consider using the workflow-level env variables directly in the matrix strategy to simplify the workflow, or add a comment explaining why this indirection is needed.
.github/workflows/build.yml
Outdated
| - name: Set up Node.js ${{ matrix.node }} | ||
| uses: actions/setup-node@v4 | ||
| uses: actions/setup-node@v6 | ||
| with: | ||
| node-version: ${{ matrix.node }} | ||
| cache: npm | ||
| if: matrix.node == env.NODE_ACTIVE_LTS_VERSION | ||
|
|
||
| # Perform this step for Node.js previous LTS version | ||
| # Continue on error to avoid failing the whole job if npm version is incompatible | ||
| - name: Set up Node.js ${{ matrix.node }} |
There was a problem hiding this comment.
The two steps on lines 38 and 47 have identical names but different behaviors. Consider giving them distinct names to make it clear which one is running in the GitHub Actions UI, such as "Set up Node.js ${{ matrix.node }} (Active LTS)" and "Set up Node.js ${{ matrix.node }} (Previous LTS)".
| env: | ||
| NODE_ACTIVE_LTS_VERSION: 24 | ||
| NODE_PREVIOUS_LTS_VERSION: 22 | ||
| NPM_VERSION: 11 |
There was a problem hiding this comment.
NPM_VERSION is set to 11, but package.json requires ^11.6.1. While npm install -g npm@11 will install the latest 11.x version (which should satisfy ^11.6.1), for clarity and to ensure the minimum version requirement is met, consider setting this to "^11.6" or "^11.6.1" to match the package.json specification.
| NPM_VERSION: 11 | |
| NPM_VERSION: ^11.6.1 |
| NODE_ACTIVE_LTS_VERSION: 24 | ||
| NODE_PREVIOUS_LTS_VERSION: 22 |
There was a problem hiding this comment.
NODE_ACTIVE_LTS_VERSION and NODE_PREVIOUS_LTS_VERSION are set to major versions only (24 and 22), but package.json devEngines requires specific minor versions (^22.11.0 || ^24.11.0). When these values are passed to setup-node, they may install versions like 22.9.0 or 24.0.0 if those are the latest available on the runner. Consider setting these to 22.11 and 24.11 to match the devEngines requirements.
| NODE_ACTIVE_LTS_VERSION: 24 | |
| NODE_PREVIOUS_LTS_VERSION: 22 | |
| NODE_ACTIVE_LTS_VERSION: 24.11 | |
| NODE_PREVIOUS_LTS_VERSION: 22.11 |
.github/workflows/build.yml
Outdated
| - name: Install npm@${{ env.NPM_VERSION }} for Node.js ${{ matrix.node }} | ||
| run: npm install -g npm@${{ env.NPM_VERSION }} | ||
| if: matrix.node == env.NODE_PREVIOUS_LTS_VERSION |
There was a problem hiding this comment.
These lines use env.NPM_VERSION and env.NODE_PREVIOUS_LTS_VERSION while the matrix values come from the prepare_env job outputs. For consistency and maintainability, consider using needs.prepare_env.outputs.NPM_VERSION and needs.prepare_env.outputs.NODE_PREVIOUS_LTS_VERSION to keep the data source consistent throughout the job.
a0e9458 to
7d1ae16
Compare
`engines` requires `node` version `>=22.11.0` instead of `>=22`, while `22.11.0` is first long term support minor version of this major version. `devEngines` requires `node` version `^22.11.0 || ^24.11.0` instead of `22.x || 24.x`, while `22.11.0` and `24.11.0` are first long term support minor versions of those major versions. `devEngines` now requires `npm` version `^11.6.1` as it is first `npm` version delivered by `node` in version `24.11.0`. We require single major version of `npm` to generate `package-lock.json` of compatible structure. This would not be possible without requiring specific version of `npm`.
7d1ae16 to
4199b8a
Compare
|
I am closing this in favor of new PR. There is so much mess generated by GH Copilot ... |
Tighten
enginesanddevEnginesenginesrequiresnodeversion>=22.11.0instead of>=22,while
22.11.0is first long term support minor version of this major version.devEnginesrequiresnodeversion^22.11.0 || ^24.11.0insteadof
22.x || 24.x, while22.11.0and24.11.0are first long term supportminor versions of those major versions.
devEnginesnow requiresnpmversion^11.6.1as it is firstnpmversion delivered bynodein version24.11.0.We require single major version of
npmto generatepackage-lock.jsonof compatible structure. This would not be possible without requiring
specific version of
npm..github/workflows/build.ymlrequires changes to install latestnpmfor previous
nodeLTS.