Skip to content

Determine Node.js version pinning policy #4258

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

Closed
jawnsy opened this issue Sep 28, 2021 · 4 comments
Closed

Determine Node.js version pinning policy #4258

jawnsy opened this issue Sep 28, 2021 · 4 comments
Labels
feature New user visible feature

Comments

@jawnsy
Copy link
Contributor

jawnsy commented Sep 28, 2021

Background

  • In order to reduce maintenance burden, it's prudent to closely track the upstream vscode project to the extent possible; in this case, the upstream behavior is ambiguous
  • We previously pinned Node.js to the major version that we support (14.x) which matches the version that VSCode requires
    • This prevents installation using yarn global add if the running version of Node is newer than the version we are using (this was reported as Installing on Termux failed due to Node version #4225
    • The subsequent fix was to relax the version requirement in fix(package.json): update node version requirements to >=14 #4230, such that either version 14 or any subsequent version, including major versions, can be used
    • We always test with the latest patch release of the 14.x series, and we do not test on other versions which match the wider constraint (currently 15.x and 16.x)
    • This means that we made an implicit assumption that Node.js would maintain backward compatibility indefinitely, or over a large enough window (several years/releases) that the constraint was unnecessary
  • Node.js has a deprecation policy and some symbols are periodically renamed or removed, which is a breaking change
  • The upstream vscode project does not specify a engines constraint or provide an installation method using npm or yarn
  • According to @code-asher, there have historically been instances where we supported Node.js 10, and due to breaking changes in the runtime, code-server was not installable on Node.js 12 - thus, breaking changes do seem to happen from time to time
  • This decision only affects users that build from source (by installing via yarn install or building code-server manually), since binary builds always include a version of Node.js 14, similar to the vscode upstream packages, which bundle a version of Node.js along with Electron

Current behavior

  • We allow installation from source with Node.js version 14, 15, or 16
  • We run all of our build and test infrastructure on version 14
  • Things seem to work with Node.js 16, but we have not tested and are not sure whether that is a reasonable expectation in general, since it relies on the Node.js maintainers avoiding breaking changes even across multiple major releases

Desired behavior

This is what I hope to determine by starting this discussion. The outcome of this could be:

  1. Status quo: we accept the risk that things may break at any point in the future, and allow installation on Node.js version >= 14

    This means that we will continue to allow users to install on Node.js 16 with no guarantee that it will continue to operate correctly and no notice in case it fails, aside from issues.

  2. Pin to major version: we revert the change in fix(package.json): update node version requirements to >=14 #4230 and require a matching major version (14.x series only).

    Users with a different Node.js version will no longer be able to install from source, but will instead need to install using the install.sh script or binary builds. This also means that users will be running the same version of Node.js that we test with.

  3. Add test infrastructure: we expand our test configuration so that we have coverage for newer versions, so that we have additional confidence that code-server runs on 15.x and 16.x

cc @jsjoeio @vapurrmaid @TeffenEllis @code-asher

@jawnsy jawnsy added the feature New user visible feature label Sep 28, 2021
@code-asher
Copy link
Member

code-asher commented Sep 28, 2021

I think we should revert and if a user really wants to use an unsupported version with yarn they can pass --ignore-engines (assuming that flag works with yarn global add, if it does not then we can remove the engines and rely on a pre-install script instead for the check).

However, I believe we also have a post-install script that errors if you use non-v14 (I think we have this because, unlike yarn, npm does not enforce engines) so we should do something about that too. I think:

  • Move to a pre-install script so you do not get the error only after you have already installed dependencies.
  • In addition to the environment variable check if engines are being ignored.

@greyscaled
Copy link
Contributor

greyscaled commented Sep 29, 2021

First off, thank you @jawnsy for the thorough, well-informed issue with a wealth of context. Next, I want to weigh in with agreement towards the sentiments proposed by @code-asher :

  • the --ignore-engines option is an opt-in to potentially breaking change, rather than us specifying a leaky/uncertain contract in engines. In addition to the changes in pre/post installs, I think we could document this option in FAQ or similar, and perhaps a comment in the fixed issue linking to this discussion.

@code-asher
Copy link
Member

Another point of data: I was trying to run code-server installed with yarn global add and I kept getting VS Code failed to load. exited unexpectedly with code 0. I was quite confused because I had been running it earlier without any issue. Turns out it was because I had switched to Node 16 for a different project and downgrading to Node 14 fixed it. We may also want a runtime check of the version...but at the very least it appears Node 16 does not work with current VS Code.

@stale
Copy link

stale bot commented May 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no activity occurs in the next 5 days.

@stale stale bot added the stale label May 1, 2022
@stale stale bot closed this as completed May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New user visible feature
Projects
None yet
Development

No branches or pull requests

4 participants