Upgrade node to v24 only for CLI#2537
Conversation
| archivePath: string, | ||
| destDir: string, | ||
| binaryName: string | ||
| ): Promise< void > { |
There was a problem hiding this comment.
These are unrelated linter fixes
| } | ||
|
|
||
| main(); | ||
| void main(); |
There was a problem hiding this comment.
Unrelated linter fix
| @@ -0,0 +1 @@ | |||
| 24.11.0 | |||
There was a problem hiding this comment.
This file could be called something else if this is not clear. I also thought about hard-coding it directly into download-node-binary but it could be slightly harder to find that way
There was a problem hiding this comment.
If it's used only in one place and is not an industry-standard file like .nvmrc, we could hardcode it there. There is no value in creating a new file format for that.
We follow the same approach with the SQLite plugin version.
There was a problem hiding this comment.
Sounds good to me, made changes in d9021ad
I also removed the fallback as it should not be needed as the version is always defined.
📊 Performance Test ResultsComparing c4b16e6 vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
Co-authored-by: Wojtek Naruniec <wojtek.naruniec@automattic.com>
ivan-ottinger
left a comment
There was a problem hiding this comment.
Thanks for the changes, Kat.
| // This is separate from .nvmrc which controls the development environment and Electron app | ||
| const BUNDLED_NODE_VERSION = 'v24.11.0'; | ||
|
|
||
| function getNodeVersion(): string { |
There was a problem hiding this comment.
I think we could now get rid of the wrapper getNodeVersion now - since it returns one constant only.
There was a problem hiding this comment.
Thanks, should be updated 👍
There was a problem hiding this comment.
Thanks for the update, Kat! Looks good! 👍🏼
…dio into fix/update-npm-in-engines
|
Connecting the dots between #2486 (comment) and this change…
We should not use a different node version for Studio CLI development compared to what we ship in the installer, since we won't be able to catch potential issues related to the node version. @wojtekn, I'll experiment with the approach we took in #2486 and try to reproduce the error you described in #2486 (comment) |
Related issues
Related to: #2486 (comment)
Proposed Changes
This PR ensures that we upgrade node version only for CLI and not for the whole Studio project. Electron is currently using v39.x and is not compatible with Node v24. I explored upgrading Electron so that CLI and the rest of the Studio project could use the same configuration but ran into a blocker with
fs-ext. Additionally, according to this comment: #2486 (comment) , it seems that our goal is to untangle with node versions used by CLI and Studio anyway.Testing Instructions
To test CLI node version:
nvm use && npm installnpm run cli:build./bin/node --versionnode v24.11.0./bin/node dist/cli/main.js --helpworks as expectedPre-merge Checklist