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

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

wants to merge 2 commits into from

Conversation

karthest
Copy link

@karthest karthest commented Sep 3, 2024

No description provided.

Copy link
Contributor

@charlespierce charlespierce left a comment

Choose a reason for hiding this comment

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

Hi @karthest, thanks for getting this conversation started! Would love to have your input on a few behavior questions that I think we want to work out to make sure this behavior is understandable and clear for users.

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.


[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?

[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.

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.

3 participants