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.

Sign up for GitHub

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

Jump to bottom

RFC for uninstalling node #53

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

RFC for uninstalling node #53

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
50a5754
RFC for uninstalling node
Sep 3, 2024
ef73a3a
update PR link
Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions text/0000-uninstall-node.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
- Feature Name: uninstall_node
- Start Date: 2024-08-21
- RFC PR:[https://github.com/volta-cli/volta/pull/1882]
- Volta Issue:

# Summary

[summary]: #summary

Add support for uninstalling node to `volta uninstall`

# Motivation

[motivation]: #motivation

Currently, Volta doesn't support uninstalling node due to this main reasons(as discussed in [#327](!https://github.com/volta-cli/volta/issues/327)): Its main purpose is to save disk space, nowadays disk size is not a big problem in many cases. So this task is in a low priority.But community has this demand since 2019, they need to manually remove some files to achieve that.Add support for uninstalling node is more convinent in this case, btw since we have `install` here we should also have `uninstall` here.

# Pedagogy

[pedagogy]: #pedagogy

The user's intuition and way of treating it are opposite to `install`. Since `install` fetch node image, unpack it and set it as default, `unintall` should remove those things.

# Details

[details]: #details

The new command will have the structure:

```
volta uninstall node@version
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the install command can accept a version "requirement", e.g. node@10 or node@latest—what it does is fetch the list of available Node versions and then resolve that to a single, specific, version before installing. How do you see uninstall working for those cases? I can see a couple of options:

  1. We require a full version, and show an error on any version-like argument that isn't actually a single version. This is the simplest approach from an implementation standpoint, but feels like it's a really bad user experience. We'd be showing errors if the user doesn't include a full version, and then requiring them to input each individual version to remove them.
  2. Read through our locally-cached versions of the tools and remove all of the versions that match the requirement. E.g. running volta uninstall node@10 would remove all cached versions of Node with major version 10. Running volta uninstall node would clean all cached versions of Node. This feels like the better user experience, though it's a bit more technically involved.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At that time I think uninstall should receive totally same args with install and treat them the same way,That means:

  1. We don't requre a full version. Just as logic in install, we support no verison, semver version , exact version and tag version.(Of course when node_index_file expires and fetch the new version, those none version, semver version and tag version will be resolved to a different one with the one when install runs. But I don't think it's a big deal)

But I agree with your second thought, that feels like a better user experience indeed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that “just support the same argument variants as install” makes a lot of sense. One note, though: it does feel like the kind of thing that might warrant a prompt, which we have yet to do anywhere. “This will remove [list all affected versions]; do you want to proceed?” It might be fine not to, but that could be a shockingly wide-ranging effect. 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree with you, a kind prompt like this is really important for users. You are soooo good at user experience.

```

totally the same with `install`.

Some examples:

```
volta uninstall node@20.16.0
```

```
volta uninstall node@lts
```

By add a `uninstall` to `Tool` trait and move uninstall logic from `Spec` to `Node, Package, Npm, Yarn, Pnpm`, it would resolve version with `node::resolve` first, and then remove tar.gz file and npm file in `home.node_inventory_dir()` and node in `home.node_image_root_dir()`

# Critique

[critique]: #critique

In fact we want `uninstall` to save disk space or just to remove those things we don't need , it seems like a command like `prune` or `clean` is more suitable, which tracks every tool's status, and when `prune` is runned, we can clean all those tools that is no longer used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good call out! I have a separate RFC up (which I'm going to be updating soon, hopefully today) to deprecate the install command and instead use a different verb: #52

In that context, would it be confusing to have a clean or prune command that works similarly to the proposed uninstall but uses a different verb? I.e. you would still need to provide a tool name (and possibly version, depending on the above discussion), but it would be volta clean node@10 instead of volta uninstall node@10

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, since install is deprecated, there is no need for a uninstall verb. How about rename uninstall to remove and support volta remove node@version to manually remove node image, no more other verbs?


In this case, maybe we should use a file to track every tool. When `volta pin` or `volta install`
is runned, the toolchain must be added to that file (means this toolchain is used in a project or is set as default), and removed outdated toolchain from that file. And we can delete all that we don't use anymore.

But this way will make some big change in volta, and how to track a toolchain's status is complex.

# Unresolved questions

[unresolved]: #unresolved-questions

1. What should `uninstall` do when uninstall a default node?
Since `install` actually fetch (in background),unpack(in background), and set as default(in user's view), `uninstall` should at least remove tar.gz file, npm file and node dir. but what `uninstall` should do when uninstall a node version that is set as default?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is an important question to resolve, though it's tricky with the overloaded behavior of install that you noted. I wonder if it would be clearer as a user with the proposed change to use pin for everything. If we did that, then we could have a command unpin that explicitly removes the pinned version, while clean or prune would remove the cached versions. That would clearly separate the two cases while making it more clear what each does.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean volta remove just focus on disk cleaning and volta unpin focus on deleting default version message(in package.json)? It looks nice.
So how about this: When user is running volta remove to remove default node version(no matter pinned in a project or set as global default one( These two cases all mean that user is trying to remove a in-using node)), we info user that they are removing a default one from disk and breake the removing process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charlespierce @karthest Yeah, I think unpin is distinctly the wrong verb for removing it from the file system. It should exactly and only, well, un-pin it, i.e. make it not-used in the current project or as a user default, at least in my view. Then, as you say, it should be a separate verb to actually clear the cached download. (I at least would find it astonishing if it deleted it.) But at that point should it even… be a thing? I cannot see a case where I would actually want to volta unpin something, only cases where I would want to volta prune or volta uninstall where uninstall means “get it off my system” rather than “remove it from the config”. 🤔 I think, anyway. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One use-case for unpin is that specifically we don't currently have a way to "unselect" a default version of Node. Once a user runs volta install node, we will effectively always be managing Node. If the user in the future wants to turn off Volta for globals and use the system default (while still maintaining Volta's behavior in pinned packages), there's no CLI way to do that. Instead, they need to go into the .volta directory and remove a file.

There seems to be less of a need for that in local projects, since the config is written into package.json and can be easily manually deleted. There's also some difficulty if we start to add support for different sources (e.g. .node-version) - in those cases we probably don't want to be deleting files / removing config from sources that aren't Volta-specific.