Use the same Node version as VSCode#1374
Merged
elenatanasoiu merged 2 commits intomainfrom Jun 13, 2022
Merged
Conversation
elenatanasoiu
force-pushed
the
elenatanasoiu/set-node-version
branch
from
1bcd800 to
1149593
Compare
3 tasks
elenatanasoiu
force-pushed
the
elenatanasoiu/set-node-version
branch
4 times, most recently
from
1acebc6 to
3f96a37
Compare
aeisenberg
reviewed
Jun 1, 2022
Contributor
There was a problem hiding this comment.
I think this is generally the right approach, but doing this means we need to be more careful to update the node version requirement when versions in vscode change.
Could you also add a note in the section on releasing in contributing.md to verify the required version of node with the version that vscode uses? And this note should specify all the places that need to be updated when there is a change.
elenatanasoiu
force-pushed
the
elenatanasoiu/set-node-version
branch
from
3f96a37 to
8eb09be
Compare
Contributor
Author
|
@aeisenberg Thanks for the review 💐 I've applied the changes you suggested. |
Contributor
|
Thanks for the changes. There's one more thing, about updating |
As recommended here #1369 (comment), we want to stay in sync with the current node version shipped with VSCode (v16.13.0): https://github.com/microsoft/vscode/blob/32d40cf44e893e87ac33ac4f08de1e5f7fe077fc/remote/.yarnrc#L2 For this we can add a `.nvmrc` file to alert nvm to switch to the preferred version automatically. It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18: #1373 We're also updating the docs to mention using `nvm` to manage node versions and point to the right place to check for current supported versions.
As recommended here #1369 (comment), since the current build for this extension does not work with Node v18 #1373, it would be good to set a maximum node version until this gets addressed. So we're updating `engines` here to allow for a maximum version, which in this case is v17.0.0.
elenatanasoiu
force-pushed
the
elenatanasoiu/set-node-version
branch
from
8eb09be to
5f21594
Compare
Contributor
Author
|
Thanks @aeisenberg, I've added an extra step here. |
aeisenberg
approved these changes
Jun 13, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As recommended here #1369 (comment), we want to stay in sync with the current node version shipped with VSCode (v16.13.0).
For this we can add a
.nvmrcfile to alert nvm to switch to the preferred version automatically.It will also help prevent builds from failing when setting up the project for the first time, as building the extension currently fails in Node v18: #1373
We're also updating the docs to mention using
nvmto manage node versions.Checklist
ready-for-doc-reviewlabel there.