Support Node.js 24 #2110
Support Node.js 24 #2110
Conversation
There was a problem hiding this comment.
Pull Request Overview
Updates the project to support Node.js version 24, addressing compatibility issues and modernizing the development environment. This includes package version updates, workflow adjustments, and specific compatibility fixes for Node.js 24 behavioral changes.
- Update Node.js version requirement to 24.0.0 across all packages and workflows
- Add custom readlink implementation to handle Windows directory symlink behavior changes in Node.js 24
- Refactor HTTP client header handling methods for improved type safety and clarity
Reviewed Changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/io/src/io-util.ts | Custom readlink implementation for Windows compatibility |
| packages/http-client/src/index.ts | Refactored header handling methods with improved type safety |
| packages/http-client/package.json | Updated Node.js engine requirement and @types/node version |
| packages/core/package.json | Updated Node.js engine requirement and @types/node version |
| packages/cache/package.json | Updated Node.js engine requirement and @types/node version |
| packages/cache/tests/saveCache.test.ts | Added missing mock and removed duplicate assertion |
| packages/artifact/tests/upload-artifact.test.ts | Fixed Stats constructor usage for Node.js 24 compatibility |
| package.json | Updated root Node.js engine requirement and @types/node version |
| .github/workflows/unit-tests.yml | Added Node.js 24.x to test matrix |
| .github/workflows/releases.yml | Updated workflow to use Node.js 24.x |
| .github/workflows/cache-windows-test.yml | Updated workflow to use Node.js 24.x |
| .github/workflows/cache-tests.yml | Updated workflow to use Node.js 24.x |
| .github/workflows/audit.yml | Updated workflow to use Node.js 24.x |
| .github/workflows/artifact-tests.yml | Updated workflow to use Node.js 24.x |
Files not reviewed (3)
- packages/cache/package-lock.json: Language not supported
- packages/core/package-lock.json: Language not supported
- packages/http-client/package-lock.json: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
packages/io/src/io-util.ts
Outdated
| return `${result}\\` | ||
| } | ||
| } catch (err) { | ||
| // If we can't access the target, just return the original result |
There was a problem hiding this comment.
Using lstat on the symlink target (result) will follow the symlink and stat the target, but result is already the resolved path. This should use lstat on the original fsPath to check if the symlink itself points to a directory.
| // If we can't access the target, just return the original result | |
| const stats = await fs.promises.lstat(fsPath) | |
| if (stats.isDirectory() && !result.endsWith('\\')) { | |
| return `${result}\\` | |
| } | |
| } catch (err) { | |
| // If we can't access the symlink, just return the original result |
There was a problem hiding this comment.
Is this suggestion related to a symlink chain? E.g. symlink -> symlink -> real
There was a problem hiding this comment.
yeah all the way through the chain to the target
There was a problem hiding this comment.
I think I might be confused by the Copilot comment. Which one resolves the full chain, lstat or stat? We want to resolve the full chain, right?
There was a problem hiding this comment.
stat resolves the whole chain
There was a problem hiding this comment.
Gotcha. Stat sounds like the right choice then? Although I guess good to test the scenario against node20 since that's the behavior we're trying to preserve, right?
| compression | ||
| ) | ||
| expect(saveCacheMock).toHaveBeenCalledTimes(1) | ||
| expect(getCompressionMock).toHaveBeenCalledTimes(1) |
There was a problem hiding this comment.
The removed duplicate assertion expect(getCompressionMock).toHaveBeenCalledTimes(1) was correct to remove as it was redundant with line 280.
packages/cache/package.json
Outdated
| "engines": { | ||
| "node": ">=24.0.0" | ||
| }, |
There was a problem hiding this comment.
This will cause any package that depends on @actions/cache to also require Node 24, removing backward compatibility.
There was a problem hiding this comment.
The plan is to have a major version bump here as well as in actions cache anyway, so any version of cache that already references the current or older versions won't be referencing this change, and any newer versions would be on the action that targets node24. What scenario do you envision needing a runner that doesn't have node 24? Self-hosted runners should all be updated and all hosted runners have node 24 already.
There was a problem hiding this comment.
Oh I can think of a scenario, such that if you were to consume the package not from the cache action. Okay I'll remove it. Thanks for the comment!
|
Since we're definitely breaking Node 18 and we don't support that version, can we just remove those tests so the CI stays green? |
|
Sure will remove those tests |
2fcb1e5 to
b0d901f
Compare
| "ts-jest": "^29.1.1", | ||
| "typescript": "^5.2.2" | ||
| }, | ||
| "overrides": { |
There was a problem hiding this comment.
I'm curious do you know why this is added?
There was a problem hiding this comment.
Bunch of them were for audit changes, simply just doing npm audit fix didn't work, so needed to override, and then url-js and node-fetch are there to stop the punycode deprecation warnings
ejahnGithub
left a comment
There was a problem hiding this comment.
LGTM for attest pkg!
| "brace-expansion": "^2.0.2", | ||
| "form-data": "^4.0.4", | ||
| "uri-js": "npm:uri-js-replace@^1.0.1", | ||
| "node-fetch": "^3.3.2" |
There was a problem hiding this comment.
- For people following along, this addresses
Azure/ms-rest-js->node-fetch@v2dependency triggers "DeprecationWarning: Thepunycodemodule is deprecated" #2173
Update the project to support Node.js version 24, including adjustments to workflows and package dependencies.
Following this PR, there will be releases, and following those there will be bumps to versions, e.g. io-utils will be released at version 2.0.0 and then that can be updated to be used in actions/cache. Following that, the actions/cache will be able to consume the package and that can be released.