Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove node 18 from CI #394

Merged
merged 3 commits into from
Feb 8, 2025
Merged

Remove node 18 from CI #394

merged 3 commits into from
Feb 8, 2025

Conversation

xuhdev
Copy link
Member

@xuhdev xuhdev commented Feb 6, 2025

Seems like the latest VSCode doesn't support Node 18 any more.

Please fill in this template.

  • Use a meaningful title for the pull request.
  • Use meaningful commit messages.
  • Run tsc w/o errors (same as npm run build).
  • Run npm run lint w/o errors.

@xuhdev
Copy link
Member Author

xuhdev commented Feb 6, 2025

@SunsetTechuila I remember you said we need to test Node 18, do you have any source? This page seems to indicate Node 20 is the minimum supported version by VSCode: https://github.com/microsoft/vscode/wiki/How-to-Contribute#prerequisites (I couldn't find the source of runtime requirement)

Seems like the latest VSCode doesn't support Node 18 any more.
@SunsetTechuila
Copy link
Member

oldest vscode supported by the extension:

"engines": {
"vscode": "^1.82.0"
},

comes with the node 18 bundled:

https://code.visualstudio.com/updates/v1_82#_update-highlights-for-nodejs

@SunsetTechuila
Copy link
Member

SunsetTechuila commented Feb 6, 2025

but the node version in the ci doesn't have to match it, that's right

@SunsetTechuila
Copy link
Member

SunsetTechuila commented Feb 6, 2025

i would do like this: SunsetTechuila@eb330b4

@xuhdev
Copy link
Member Author

xuhdev commented Feb 7, 2025

I think it's OK to test multiple Node versions. By doing so, we know we are ready when VSCode upgrades Node version.

Since 1.90, released in May 2024, VSCode has moved to Node 20: https://code.visualstudio.com/updates/v1_90

It looks like VSCode doesn't have an LTS branch and I don't know if we should support a version as old as 1.82.

@SunsetTechuila Are you stuck on an old version for some reason? I just would like to understand a bit more about how dated VSCode we should support.

(Anyway, I would release 0.17 first before merging this PR. Node 18 is EOL in April. Even if we release a newer version before April, people who are stuck on Node 18 can still install 0.17 and should upgrade in April.)

@SunsetTechuila
Copy link
Member

SunsetTechuila commented Feb 7, 2025

I think it's OK to test multiple Node versions. By doing so, we know we are ready when VSCode upgrades Node version.

Node in the CI is used only to:

  1. Install dependencies
  2. Run tools like typescript and eslint
  3. Execute the runTests function of the @vscode/test-electron package

The tests themselves are run inside the latest VSCode with the Node bundled with it

@SunsetTechuila Are you stuck on an old version for some reason? I just would like to understand a bit more about how dated VSCode we should support.

I'm all for backward compatibility as long as it doesn't hurt the dev experience. I think this is one such case - we are not using anything that requires newer VSCode

@xuhdev
Copy link
Member Author

xuhdev commented Feb 7, 2025

I see -- it makes sense since they are for dev experience only. Since we are not using features that are available to Node 20+ anyway, let's merge this only after Node 18 is EOL! 👍

@SunsetTechuila
Copy link
Member

I'm not sure we understood each other. I don't see any reason not to use only the latest Node version in the CI, and I don't see any reason to bump the required VSCode version any time soon.

@xuhdev
Copy link
Member Author

xuhdev commented Feb 8, 2025

I'm a bit confused too, because you seem to suggest Node 20 but the latest LTS is 22.

If there's no special reason, I would bump development requirement to latest Node LTS (22), which has the most recent features and devs can always install with nvm.

If we agree on this, I can change the PR to support latest Node LTS as a development requirement.

@SunsetTechuila
Copy link
Member

I'm a bit confused too, because you seem to suggest Node 20 but the latest LTS is 22

I don't. Have you checked my commit? This is the change I want to make to your PR before merging

I would bump development requirement to latest Node LTS (22)

If you mean Node types, then they should match the Node version bundled with the oldest supported VSCode

@xuhdev
Copy link
Member Author

xuhdev commented Feb 8, 2025

Got it. I've appended your suggested change to this PR.

One known problem is that the test code should follow Node LTS's type definition, but it's a minor issue that we can stand.

I'm merging now, but feel free to create new PRs to change anything unsatisfying.

@xuhdev xuhdev merged commit ec849f9 into main Feb 8, 2025
3 checks passed
@xuhdev xuhdev deleted the 18 branch February 8, 2025 05:02
@SunsetTechuila
Copy link
Member

Maybe I should say loudly what I always had in mind - Node LTS releases are almost always fully backwards compatible. So the only problem I see with the tests rn is that we don't run them against VSCode 1.82

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