Conversation
Many jobs aren't testing compatibility with the Node version and should be expected to work with the default
|
There was a problem hiding this comment.
Pull request overview
This PR updates GitHub Actions workflows to stop testing against Node.js 18 (now EOL) and adjusts CI matrices accordingly.
Changes:
- Removes Node 18 from the main CI test matrices.
- Updates several workflows’
actions/setup-nodesteps (including depcheck, release, browser tests, and action-pin automation). - Simplifies some step names related to Node setup.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/ci.yml | Drops Node 18 from some matrices and adjusts multiple setup-node usages. |
| .github/workflows/depcheck.yml | Removes Node 18 from the depcheck workflow’s Node setup. |
| .github/workflows/browser-test.yml | Removes Node 18 from browser-test workflow’s Node setup. |
| .github/workflows/release.yml | Removes pinned Node version from release workflow’s Node setup. |
| .github/workflows/update-action-pins.yml | Removes pinned Node version from weekly action-pin updater workflow. |
| .github/workflows/update-action-pins-major.yml | Removes pinned Node version from monthly major action-pin updater workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -36,7 +36,6 @@ jobs: | |||
| - name: Use Node.js | |||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | |||
| with: | |||
There was a problem hiding this comment.
actions/setup-node is invoked without providing node-version (or node-version-file). setup-node expects one of these inputs and will error, which would break the lint job. Add an explicit Node version (e.g. the previously used 22.x) or switch to node-version-file, or remove this step if you intend to use the runner’s default Node.
| with: | |
| with: | |
| node-version: 22.x |
There was a problem hiding this comment.
I think you're wrong. The job https://github.com/endojs/endo/actions/runs/21963603815/job/63447382387?pr=3084 shows actions/setup-node working without a node-version argument. It picked v20. It could be v22 but I don't really care. Unless it's a matrix job, we're not testing Node version integration
There was a problem hiding this comment.
If we're not using caching or anything, and the Node version doesn't matter, then we can just omit actions/setup-node altogether and use the runner's default version.
| - name: Use Node.js | ||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | ||
| with: | ||
| node-version: 22.x | ||
| cache: yarn | ||
|
|
There was a problem hiding this comment.
actions/setup-node is invoked without providing node-version (or node-version-file). This will cause the docs-markdown job to fail during Node setup. Specify a Node version (e.g. 22.x as before) or use node-version-file.
| matrix: | ||
| node-version: [18.x, 20.x, 22.x] | ||
| node-version: [20.x, 22.x] | ||
| platform: [ubuntu-latest, macos-15] |
There was a problem hiding this comment.
The PR aims to drop Node 18, but this workflow still includes a test-async-hooks matrix entry for Node 18 elsewhere in this file. Remove that entry as well so CI no longer runs on Node 18.
| - name: Use Node.js | ||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | ||
| with: | ||
| node-version: 22.x | ||
| cache: yarn | ||
|
|
There was a problem hiding this comment.
actions/setup-node is invoked without node-version (or node-version-file) in the test-hermes job. This is likely to fail at runtime; specify the intended Node version (previously 22.x) or use a version file.
| - name: Use Node.js 20.x | ||
| - name: Use Node.js | ||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | ||
| with: |
There was a problem hiding this comment.
actions/setup-node is invoked without node-version (or node-version-file) in the test-ocapn-python job. This can break CI; specify an explicit version (e.g. 20.x to match the rest of the workflow) or use node-version-file.
| with: | |
| with: | |
| node-version: '20.x' |
| - name: Setup Node.js 20 | ||
| - name: Setup Node.js | ||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | ||
| with: |
There was a problem hiding this comment.
actions/setup-node is invoked without node-version (or node-version-file). This will likely fail the release workflow at the Node setup step. Specify the intended Node version (previously 22.x) or use a version file.
| with: | |
| with: | |
| node-version: '22.x' |
| @@ -24,7 +24,6 @@ jobs: | |||
| - name: Use Node.js | |||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | |||
| with: | |||
There was a problem hiding this comment.
actions/setup-node is invoked without node-version (or node-version-file) after removing the 22.x pin. This will likely break this automation workflow; specify a Node version or use node-version-file.
| with: | |
| with: | |
| node-version: '22.x' |
| - name: Use Node.js 22.x | ||
| - name: Use Node.js | ||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | ||
| with: |
There was a problem hiding this comment.
actions/setup-node is invoked without node-version (or node-version-file) in the check-action-pins job. This will likely fail; specify a Node version or remove the step if you want to rely on the runner’s preinstalled Node.
| with: | |
| with: | |
| node-version: 20.x |
| @@ -33,8 +33,6 @@ jobs: | |||
| run: mv /opt/google/chrome /opt/google/chrome-unstable | |||
| - uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4 | |||
| - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | |||
There was a problem hiding this comment.
actions/setup-node is invoked without node-version (or node-version-file) after removing the Node 18 configuration. This will likely fail the browser test workflow; specify the supported Node version (e.g. 20.x) for reproducible runs.
| - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | |
| - uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | |
| with: | |
| node-version: '20.x' |
| @@ -24,7 +24,6 @@ jobs: | |||
| - name: Use Node.js | |||
| uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6 | |||
| with: | |||
There was a problem hiding this comment.
actions/setup-node is invoked without node-version (or node-version-file) after removing the 22.x pin. This will likely break this automation workflow; specify a Node version or use node-version-file.
| with: | |
| with: | |
| node-version: 22.x |
_incidental_ ## Description There was some confusion recently about the API of `BundleCache`. This adds docs for it. It also adds tests coverage, fixes up types and some runtime cleanup. The new tests fail under Node 18 because it defaults to CJS for `.js` files. ~Since that version is past EOL this also removes it from CI matrix. And while searching it unpins `node-version` from jobs which aren't meant to test release compatibility (and should always work with latest default Node version).~ This provides a package.json and defers CI changes to #3084 ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations This should go out in a release so it needs a changeset. Patch release I suppose. ### Testing Considerations CI ### Compatibility Considerations no, just docs and types and some small bug fixes ### Upgrade Considerations will address in a changeset file
incidental
Description
Node 18 is past EOL.
We don't need it for browser testing either: #3085 (comment)
Security Considerations
stop testing EOL
Scaling Considerations
none
Documentation Considerations
engines aren't updated yet in this PR, assuming that it still works under Node 18 even though we stop testing. Open to updating the engines line. (Though it wasn't when dropping Node 16)
Testing Considerations
CI
Compatibility Considerations
drops support for Node 18
Upgrade Considerations
needs a changeset entry. will wait for approval of the overall change before doing more work.