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

Fish 3.0 #122

Closed
jorgebucaran opened this issue Nov 14, 2020 · 75 comments · Fixed by #123
Closed

Fish 3.0 #122

jorgebucaran opened this issue Nov 14, 2020 · 75 comments · Fixed by #123
Labels
meta Questions, news and communication

Comments

@jorgebucaran
Copy link
Owner

jorgebucaran commented Nov 14, 2020

I'm working on a new, better nvm.fish, with a more consistent command-line interface (#81, #111), better XDG support (#108), and Fish 3.0 in mind. I also plan to fix long standing issues (#101, #112), so that switching versions only modifies the current shell. Here's the what I have in mind. All pretty straight-forward. Please let me know what we're missing and what else would you like to see?

Install the specified Node.js VERSION into $XDG_DATA_HOME.

nvm install VERSION

Activate the specified Node.js version. Automatically install the version if it isn't already installed.

nvm use VERSION

Use the version specified in the nearest .nvmrc or .node-version file, walking up the directory tree from the current working directory.

nvm use

Print the Node.js version currently in use.

nvm current

List all installed Node.js versions.

nvm list

List Node.js versions you can install.

nvm list-remote

Remove the specified Node.js version.

nvm remove VERSION
@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 14, 2020

If you wish to switch Node.js versions automatically when $PWD changes, creating a configuration snippet or adding this code to your config.fish should be simple enough.

function _nvm_on_pwd --on-variable PWD 
    test -e $PWD/.nvmrc || test -e $PWD/.node-version && nvm use
end

Maybe we can offer this out of the box without degrading shell performance. Need to think about it more.

@thernstig
Copy link

thernstig commented Nov 15, 2020

This sounds great! I assume nvm use will also install the specified version in .nvmrc or .node-version if not already installed? And what happens if neither of the files is found when walking up the directory tree?

@jorgebucaran
Copy link
Owner Author

Yes, to the first question. And do nothing, to the second one. What do you think?

@thernstig
Copy link

I'd probably expect it to talk up at most 10 levels and stop looking after that. It should then probably print a message saying that it did not found any of the files from the current directory and 10 levels up.

@jorgebucaran
Copy link
Owner Author

Too arbitrary? I don't really see the issue with infinite levels. How deep could you be anyway?

Do you know what nvm-sh does by the way?

@ljharb
Copy link

ljharb commented Nov 16, 2020

nvm looks up forever until it hits /, or finds an nvmrc file. When none is found, it uses the “default” alias, or if that doesn’t exist, it errors.

@jorgebucaran
Copy link
Owner Author

Thanks, @ljharb. We should do that too. That's also what we currently do.

Do we also want an nvm alias command like in nvm-sh? cc @jackwestmoretab @thernstig @andreiborisov

@jackwestmoretab
Copy link

@jorgebucaran you mean, to allow the user to set their own aliases?

@jorgebucaran
Copy link
Owner Author

Correct. I've always found that part, let's say "unintuitive" about nvm-sh, but that might be just me.

@jackwestmoretab
Copy link

I personally just use the built-in aliases (e.g. system), so have no investment in setting my own :)

Worth seeing what others think, though!

@thernstig
Copy link

thernstig commented Nov 16, 2020

I personally do not care for aliases, so I cannot see the use case. system does make sense though. But each to his own I suppose. Maybe hold it off until after 2.0 has hit, if someone requests it?

@jackwestmoretab
Copy link

Maybe hold it of until after 2.0 has hit, if someone requests it?

Sounds sensible to me. And the important issue here IMHO is fixing the set -g/-U ... behaviour.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 16, 2020

Okay. We're just going to prepend to the PATH on nvm use. Now, do we also want to prepend to the PATH on shell start? And if so, which version do we want to prepend to it? Say I have two tabs open, in one I used nvm use 14, in the other, nvm use 15. Now, I open a new tab. What does node -v print?

@ljharb
Copy link

ljharb commented Nov 16, 2020

In nvm, using doesn't change or set the default - however when you install a node version when none existed, it will set the default if none exists.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 16, 2020

So node -v will print 14.x.x in the new tab for the scenario I described above.

For nvm.fish 2.0, I'd prefer changing the default version every time you do nvm use VERSION. That means using 2.0 would be very much like using 1.x for people that just like to switch node globally, but without breaking existing sessions (or sub-shells they may spawn) using a different node.

Hope that makes sense.

@andreiborisov
Copy link

andreiborisov commented Nov 16, 2020

No, please don’t mess with the global node version. If you have personal fish scripts or some other stuff that use node (I have) they will break. Changing the global version should be either via a separate subcommand or not present at all.

The whole point of NVM is to use different Node versions side by side.

I’m also don’t understand why would you even need to change the global version? There are brew and other package managers that do the job just fine. Check for .nvmrc on shell startup, if nothing found, do not append PATH. It will contain your global node version, that you installed via brew, then. Simple and predictable.

@andreiborisov
Copy link

andreiborisov commented Nov 16, 2020

Also, a subcommand akin to npm install --save-dev that would create .nvmrc for you would be quite awesome to have. I mean, you’ll want to add .nvmrc every time you set up a new project and it should be quite trivial to implement.

For example:

nvm use 10 --save

@jorgebucaran
Copy link
Owner Author

@andreiborisov Did you read this part?

...but without breaking existing sessions (or sub-shells they may spawn) using a different node.

Imagine nvm-sh changed the default alias whenever people run nvm use VERSION. That's what I am planning to do. Why isn't that good?

@andreiborisov
Copy link

andreiborisov commented Nov 16, 2020

Sub-shells and other sessions are only part of the problem.

Let’s say you have an automatically run fish script that calls node. It expects one version, that you globally installed. You’ve worked on some Node projects, default nvm node version has changed. node version for that script changed too. It breaks.

Another thing to consider: what if you want to keep your default Node installation always up-to-date to have all security updates as fast as they land? With the brew-installed version it’s easy because it’s a system package manager. But now you have a pinned nvm-installed version by default. How do you keep it up-to-date?

The point is, the global installation has different requirements and use-cases.

And finally, it’s easier and more predictable. You global installation = your default. Why it matters? Let’s say you’ve worked on some Node projects last week. Which is your default node version now? You can’t be sure, you need to run nvm current. There is no even need for this command in my proposal.

Which brings me to: it’s easier to implement and maintain. You don’t need to store which one is the default. You check dynamically on nvm use or startup, you append PATH. That’s it.

@andreiborisov
Copy link

andreiborisov commented Nov 16, 2020

Regarding the depth of checking for .nvmrc: I suggest going up to the nearest Git repo. Checking for .git folder should be enough.

@jorgebucaran
Copy link
Owner Author

@andreiborisov Have you used either nvm or nvm.fish?

@andreiborisov
Copy link

andreiborisov commented Nov 16, 2020

I’ve used nvm.fish (it broke my system-wide scripts back then), but not familiar with the original nvm.

@andreiborisov
Copy link

andreiborisov commented Nov 16, 2020

Did I get something wrong about your proposed implementation? I've assumed you want to store the current node version and switch to it on fish startup automatically by appending PATH.

@jorgebucaran
Copy link
Owner Author

Thanks for clarifying that. Let me just clarify that we won't touch your /usr/local/bin/node or replace binaries, we are just going to change the $PATH.

Now, what about functioning the same as nvm-sh?

@jorgebucaran
Copy link
Owner Author

You global installation = your default. Why it matters? Let’s say you’ve worked on some Node projects last week. Which is your default node version now?

What do we do if there's no system node installed? Doing the same as nvm-sh makes sense. See here.

@andreiborisov
Copy link

andreiborisov commented Nov 16, 2020

Let me just clarify that we won't touch your /usr/local/bin/node or replace binaries, we are just going to change the $PATH.

That's exactly the issue. Fish will run nvm.fish before executing my system-wide fish scripts and the version of node will effectively change for them too. I'm talking about scripts like this:

#!/usr/local/bin/fish

set log 'path/to/logs'
node app.js >> $log

This will not call brew-installed version.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 16, 2020

@andreiborisov Yeah, which is why I asked what about we function like nvm-sh? It's easier to implement and there's no need to run anything on startup, except for #122 (comment), but that's not so bad.

@andreiborisov
Copy link

andreiborisov commented Nov 16, 2020

What do we do if there's no system node installed? Doing the same as nvm-sh makes sense.

I suggest doing nothing. 99% of people who use Node have it already installed. In my opinion, if your project needs a specific version you'll create .nvmrc or run nmv use explicitly (assuming you going to read .nvmrc on startup).

That being said you can check for that case specifically like nvm-sh, I see no harm in that, aside from maybe it's being confusing in some circumstances. Mainly how it should behave if the user installs nvm first and global node after that? Is it going to ditch the nvm default?

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 16, 2020

Mainly how it should behave if the user installs nvm first and global node after that? Is it going to ditch the nvm default?

Think it would. My understanding is that nvm-sh prepends the default (alias) node to the PATH.

default and system are different things.

@andreiborisov
Copy link

andreiborisov commented Nov 18, 2020

Thanks! I wouldn't have a problem with that. @andreiborisov you against this, right? Just checking.

Yes, I’m against it, since it will break system-wide scripts.

The main reason why I think nvm-sh is designed that way is for users not to type nvm use every session. Since we’ll have auto-loading of .nvmrc it won’t be a problem (I’m currently benchmarking synchronous solution to see if we even need to do async variant, I have a pretty good idea on how to implement it though). If you’re wary about this design decision, let me done autoloading first. In my opinion with autoloading it’s going to be a superior design compared to nvm-sh and we won’t need to set the default alias.

@jorgebucaran
Copy link
Owner Author

@andreiborisov I'm not sure if autoloading immediately when changing directories will be a default feature, but I do want to make this easy for users. I might not use it, but I'm willing to be convinced this is worth it.

I have an update as well. I want to remove the nvm use command. I think it's useless. Install/use, it's the same. Just use nvm install VERSION. Objections?

@andreiborisov
Copy link

andreiborisov commented Nov 18, 2020

I want to remove the nvm use command. I think it's useless. Install/use, it's the same. Just use nvm install VERSION. Objections?

👍 nvm install also like npm install, totally makes sense.

I would’ve removed nvm current too, since, without aliases, it is basically node -v.

@ljharb
Copy link

ljharb commented Nov 18, 2020

There is a huge difference; just because you want to use a version doesn’t mean you have the bandwidth, cpu time, human clock time, or desire to install it.

@jorgebucaran
Copy link
Owner Author

Frankly, I don't understand this point. We just need to download the version once and prepend it to the PATH. It's virtually instantaneous.

@ljharb
Copy link

ljharb commented Nov 18, 2020

Many people pay for bandwidth; many people use a version manager in CI, or on internet appliances that have very low cpu power; some may be using it in a docker container that has versions preinstalled and permissions locked down so nothing new can be installed. Additionally, many systems require compilation, and a mere download isn’t sufficient.

@andreiborisov
Copy link

👍 nvm install also like npm install, totally makes sense.

After giving more thoughts I’ve realized that nvm use may be a better name for it or otherwise it’ll be awkward when you use it on subsequent invocations since you won’t install anything then.

@ljharb
Copy link

ljharb commented Nov 18, 2020

Specifically, it’s very important for any command that a user knows, with certainty, whether it will require network access, and whether it will be potentially destructive (ie, make persistent changes). nvm install is both, nvm use is neither.

At the very least, if you’re going to do this, please pick a command name that won’t be confusing - ie, something that’s neither use nor install.

@andreiborisov
Copy link

There is a huge difference; just because you want to use a version doesn’t mean you have the bandwidth, cpu time, human clock time, or desire to install it.

I prefer it to be automatic. Seems like a rarely encountered edge case...

Maybe something like nvm cleanup that would delete old distributions will prove to be useful or perhaps a setting to limit storage space/number of distros? How much space one Node distro eats up anyway?

@ljharb
Copy link

ljharb commented Nov 18, 2020

The mere download could cost the user money - not everyone lives in a country where internet is effectively unlimited. Disk space isn’t really my concern (which is why i didn’t mention it); disk space is effectively infinite and free at this point.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 18, 2020

...it’ll be awkward when you use it on subsequent invocations since you won’t install anything then.

Maybe you are fluxing "install" and "download" into one, but that's not always the case. In Fisher, fisher install X updates X if it's already installed. I don't find this awkward, but I concede opinion may vary. It's certainly not worse than having an additional nvm use command.

At the very least, if you’re going to do this, please pick a command name that won’t be confusing - ie, something that’s neither use nor install.

There's no need to pick a different name. The proposal is not about changing how nvm install works, is about removing nvm use. This nvm install will be effectively the same as nvm-sh nvm install, which installs (if not already installed) and uses/activates the specified version.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 18, 2020

The mere download could cost the user money - not everyone lives in a country where internet is effectively unlimited. Disk space isn’t really my concern (which is why i didn’t mention it); disk space is effectively infinite and free at this point.

Is this on-topic? Sorry, I don't think we're really arguing about that. Maybe it's a reply to #122 (comment). Not sure. FWIW I'm definitely not arguing against that. 👍

...some may be using it in a docker container that has versions preinstalled...

If Node is already pre-installed/downloaded, nvm install behaves as nvm use.

Additionally, many systems require compilation, and a mere download isn’t sufficient.

People on those systems don't use Fish. We've never supported compilation AFAIR. It's a decent compromise.

@jorgebucaran
Copy link
Owner Author

BTW I'm also planning to do this. From the new documentation:


The nvm install command activates the specified Node version only in the current environment. If you want to set the default version for new shells use:

set -U nvm_default_node 12.9.1

Thoughts? @andreiborisov @thernstig @jackwestmoretab

@andreiborisov
Copy link

Thoughts?

Looks good🙏

@jackwestmoretab
Copy link

Thoughts?

Looks good to me 👍

Thanks for taking the time over this, @jorgebucaran!

@jorgebucaran
Copy link
Owner Author

PR: #123 🙏

@thernstig
Copy link

I had a few comments that I did not think of before, but I suppose those were better left as comments in the PR. So that's where they are 😃

@jorgebucaran
Copy link
Owner Author

Does anyone want to join me for a good old bikeshedding session?

スクリーンショット 2020-11-26 20 00 20

@thernstig
Copy link

thernstig commented Nov 26, 2020

I have no idea which one of those are the best - the more I stare at them the more I keep changing my opinion 😝 Would an "already installed" be too verbose? (My opinion: Yes it would be too verbose)

@andreiborisov
Copy link

andreiborisov commented Nov 26, 2020

I would say, the first is a clear winner.

Checkmark is an unambiguous indicator that the version is present/available/installed/downloaded. The arrow does imply downloading specifically, but it often used as an action icon, so it can be mistaken as a call to action rather than a status:
CleanShot 2020-11-26 at 17 13 17@2x
CleanShot 2020-11-26 at 17 14 24@2x

@jackwestmoretab
Copy link

I would say, the first is a clear winner.

I agree 100%.

@jorgebucaran jorgebucaran changed the title nvm.fish 2.0 Rewrite Nov 26, 2020
@jorgebucaran jorgebucaran linked a pull request Dec 5, 2020 that will close this issue
@thernstig
Copy link

@jorgebucaran maybe #103 could be unpinned now?
And does the other pinned "#82" need a slight re-write?

@jorgebucaran
Copy link
Owner Author

Done! Bullet list rewritten for 2.0. Also added a note! Thanks for reminding me of that. 💯

@jorgebucaran jorgebucaran changed the title Rewrite Fish 3.0 Jan 16, 2021
@jorgebucaran jorgebucaran added meta Questions, news and communication and removed Discussion labels Jan 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Questions, news and communication
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants