-
Notifications
You must be signed in to change notification settings - Fork 7
Tighten engines and devEngines
#682
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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", |
Copilot
AI
Dec 17, 2025
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Copilot
AI
Dec 17, 2025
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.
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 }} |
Copilot
AI
Dec 17, 2025
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.
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 |
Copilot
AI
Dec 17, 2025
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.
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 |
Copilot
AI
Dec 17, 2025
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.
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 |
Copilot
AI
Dec 17, 2025
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.
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.