Skip to content

Conversation

kelson42
Copy link
Contributor

@kelson42 kelson42 commented May 8, 2025

  • Remove Node.js 18 in CI
  • Add Node.js 24 in CI
  • Move CD to Node.js 20
  • Expicacit Node.js version deps in package.json

@kelson42 kelson42 added this to the 3.5.0 milestone May 8, 2025
@kelson42 kelson42 marked this pull request as draft May 8, 2025 07:54
@kelson42 kelson42 marked this pull request as ready for review May 8, 2025 07:59
@kelson42 kelson42 requested review from Copilot and benoit74 May 8, 2025 07:59
Copy link

@Copilot 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 adds Node.js 24 to the CI setup to ensure the application is tested on the latest Node.js version.

  • Updated the node matrix in the CI workflow to include version 24.

Copy link
Contributor

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should remove support for Node 18 in the same move, since this version is now out of maintenance.

CD should also be updated to use Node 20 now.

And I'm surprised we do not force engine versions in package.json:

  "engines": {
    "node": ">=20.0.0"
  },

While not strictly mandatory, I'm pretty sure this would help save users complaining / opening issues linked to old unmaintained Node versions. At least this is the policy we follow for python-libzim.

@kelson42 kelson42 force-pushed the kelson42-patch-1 branch from a1c8375 to fff500a Compare May 11, 2025 05:33
@kelson42 kelson42 requested review from benoit74 and Copilot May 11, 2025 05:33
Copy link

@Copilot 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 updates the Node.js versions used across CI and CD pipelines and explicitly declares Node.js version dependencies in package.json.

  • Update package.json to set the "engines" field to support Node.js versions 20 through 24.
  • Modify .github/workflows/ci.yml to remove Node.js 18 and add Node.js 24 to the test matrix.
  • Update .github/workflows/cd.yml to switch the deployment environment from Node.js 18.x to Node.js 20.x.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
package.json Added "engines" field with Node.js version range ">=20 <=24".
.github/workflows/ci.yml Updated the CI matrix to test with Node.js versions 20, 22, and 24.
.github/workflows/cd.yml Updated deployment to use Node.js 20.x instead of 18.x.

Copy link
Contributor

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I almost forgot about it, but we should add an npm configuration file to at least enforce the version on dev workstations, just like we did on mwoffliner: https://github.com/openzim/mwoffliner/blob/main/.npmrc

@kelson42 kelson42 changed the title Add Node.js 24 to the CI Add Node.js 24 support in replacement of Node.js 18 May 16, 2025
@kelson42 kelson42 changed the title Add Node.js 24 support in replacement of Node.js 18 Increment Node.js supported versions (-18.x +24.x) May 16, 2025
@kelson42 kelson42 requested review from Copilot and benoit74 May 16, 2025 13:51
Copy link

@Copilot 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 updates the supported Node.js versions across the project by removing 18.x, adding 24.x, moving CD to use 20.x, and explicitly declaring engine requirements.

  • Add an explicit engines field in package.json for Node.js ≥20 and ≤24
  • Enable engine-strict in .npmrc
  • Update CI matrix to test Node.js 20, 22, and 24
  • Update CD workflow to run on Node.js 20.x

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

File Description
package.json Add "engines": { "node": ">=20 <=24" }
.npmrc Enable engine-strict=true
.github/workflows/ci.yml Remove Node 18 from matrix and add Node 24
.github/workflows/cd.yml Switch CD step from Node 18.x to Node 20.x
Comments suppressed due to low confidence (1)

.npmrc:1

  • [nitpick] Add a note in the README or CONTRIBUTING guide explaining that engine-strict=true is enforced so contributors understand the Node.js version requirement upfront.
engine-strict=true

Copy link
Contributor

@benoit74 benoit74 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kelson42 kelson42 merged commit f86e1ef into main May 17, 2025
30 of 31 checks passed
@kelson42 kelson42 deleted the kelson42-patch-1 branch May 17, 2025 18:11
Copy link

codecov bot commented May 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (789030a) to head (332fc7b).
Report is 7 commits behind head on main.

Additional details and impacted files
@@     Coverage Diff     @@
##   main   #162   +/-   ##
===========================
===========================

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants