Skip to content

Conversation

@bedrich-schindler
Copy link
Contributor

@bedrich-schindler bedrich-schindler commented Dec 17, 2025

Tighten engines and devEngines

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.

.github/workflows/build.yml requires changes to install latest npm
for previous node LTS.

`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`.
Copy link

Copilot AI left a 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.node from >=22 to >=22.11.0 to require the first LTS minor version
  • Changed devEngines.runtime.version from 22.x || 24.x to ^22.11.0 || ^24.11.0 for stricter LTS-only development
  • Added devEngines.packageManager.version requirement of ^11.6.1 for 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.

Comment on lines +32 to +37
"node": ">=22.11.0"
},
"devEngines": {
"runtime": {
"name": "node",
"version": "22.x || 24.x",
"version": "^22.11.0 || ^24.11.0",
Copy link

Copilot AI Dec 17, 2025

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:

  1. Aligning devEngines to match engines by using >=22.11.0 <25.0.0 (if 24.x is the max supported), or
  2. Tightening engines to match the actual supported versions if only 22.x and 24.x LTS lines are intended to be supported.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

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.

@bedrich-schindler bedrich-schindler force-pushed the task/tighten-engines branch 12 times, most recently from cbcd652 to b1e6d73 Compare December 17, 2025 21:42
NPM_VERSION: 11

jobs:
prepare_env:
Copy link
Contributor Author

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.

node-version: ${{ matrix.node }}
cache: npm
if: matrix.node == env.NODE_PREVIOUS_LTS_VERSION
continue-on-error: true
Copy link
Contributor Author

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.

Copy link

Copilot AI left a 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.

Copy link

Copilot AI left a 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.

Comment on lines 11 to 23
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
Copy link

Copilot AI Dec 17, 2025

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.

Copilot uses AI. Check for mistakes.
Comment on lines 16 to 47
- 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 }}
Copy link

Copilot AI Dec 17, 2025

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)".

Copilot uses AI. Check for mistakes.
env:
NODE_ACTIVE_LTS_VERSION: 24
NODE_PREVIOUS_LTS_VERSION: 22
NPM_VERSION: 11
Copy link

Copilot AI Dec 17, 2025

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.

Suggested change
NPM_VERSION: 11
NPM_VERSION: ^11.6.1

Copilot uses AI. Check for mistakes.
Comment on lines +6 to +7
NODE_ACTIVE_LTS_VERSION: 24
NODE_PREVIOUS_LTS_VERSION: 22
Copy link

Copilot AI Dec 17, 2025

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.

Suggested change
NODE_ACTIVE_LTS_VERSION: 24
NODE_PREVIOUS_LTS_VERSION: 22
NODE_ACTIVE_LTS_VERSION: 24.11
NODE_PREVIOUS_LTS_VERSION: 22.11

Copilot uses AI. Check for mistakes.
Comment on lines 56 to 58
- 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
Copy link

Copilot AI Dec 17, 2025

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.

Copilot uses AI. Check for mistakes.
@bedrich-schindler bedrich-schindler force-pushed the task/tighten-engines branch 2 times, most recently from a0e9458 to 7d1ae16 Compare December 18, 2025 13:11
`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`.
@bedrich-schindler
Copy link
Contributor Author

I am closing this in favor of new PR. There is so much mess generated by GH Copilot ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants