jorgebucaran / nvm.fish

The Node.js version manager you'll adore, crafted just for Fish
https://git.io/nvm.fish
MIT License
2.08k stars 69 forks source link

RFC: Automatic version switching #132

Closed jorgebucaran closed 3 years ago

jorgebucaran commented 3 years ago

It would pretty awesome if when you cd'ed into a directory with an .nvmrc or .node-version file, we automatically detected it and nvm use (or nvm install?) for you. We don't want to slow down the shell the slightest, so this must happen in the background.

ljharb commented 3 years ago

If it’s async, that sounds like it’d be a race condition between the right node version loading, and me trying to run a command that needs node/npm.

jorgebucaran commented 3 years ago

Ugh, I just did not think this through entirely. You're correct. Running nvm in the background would be a total disaster. My plan was only to detect the version file in the background, but I'm not even sure I want to pursue that anymore.

Here's a different angle on the same idea. Introduce a new variable, e.g. $nvm_path where you stuff the paths you want to keep track of. So, instead of looking for a version file every time you cd somewhere, we nvm use only when going into one of those directories.

We can implement this by watching PWD.

function nvm_pwd --on-variable PWD 
    contains -- $PWD $nvm_path && nvm use
end

The cost of running contains is negligible. Worst performance for > 100 paths:

$ time contains -- ~/my/project (echo -s /(random)(seq 100){a,b,c,d,e,f,g,\n}) ~/my/project
________________________________________________________
Executed in    8.00 micros    fish           external
   usr time    7.00 micros    7.00 micros    0.00 micros
   sys time    3.00 micros    3.00 micros    0.00 micros

cc @thernstig @andreiborisov

andreiborisov commented 3 years ago

I’m not sure if the race condition is a problem in this case. Isn’t nvm use pretty much instantaneous? Are you realistically going to be able to type node command before it completes in the background?

ljharb commented 3 years ago

When my prompt includes a node command, absolutely.

andreiborisov commented 3 years ago

Hmm, yes, I didn’t think of that...

Maybe it should be provided as a possible config then? Surely there is a significant subset of people who don't use node in their prompt and would prefer the async variant.

I’m also fine with restricting synchronous variant for certain user-defined folders as Jorge suggested.

thernstig commented 3 years ago

It would be cool if it worked without having to config paths by doing test -e <file>. Does it invoke a large penalty? I'd recommend testing with hyperfine for comparison: https://github.com/sharkdp/hyperfine

You could make it configurable possibly? I mean, there is a lot of stuff already happening on path change (updating the prompt with fish_prompt) and I also use direnv to check every path change and I don't think any of that is noticeable.

jorgebucaran commented 3 years ago

I'm not using test -e anywhere in the example above.

thernstig commented 3 years ago

Maybe I was unclear. I meant does it invoke a large penalty to do test -e .nvmrc everytime, instead of contains -- $PWD $nvm_path?

jorgebucaran commented 3 years ago

I don't know, I didn't test it. Probably worse than using contains for sure.

thernstig commented 3 years ago

Any chance you would want to test it? Or do you think it is a horrible idea altogether, not worth testing?

jorgebucaran commented 3 years ago

The issue is that test -e .nvmrc -o -e .node-version is not good enough, you need to find up recursively.

https://github.com/jorgebucaran/nvm.fish/blob/9461f8049b83febe36e8aeabe54cfbb0673d2dda/functions/nvm.fish#L156-L161

That's why I proposed introducing a variable instead. With a variable, we check if PWD is in the user-defined list of paths and only run nvm use then.

jorgebucaran commented 3 years ago

I don't think I will be introducing a new variable right now and this hasn't received a reply in a while, so I'm going to close.

aral commented 3 years ago

Any chance we can re-open this. My expectation would be that at the very least I’m prompted if I enter a directory with an .nvmrc file that I don’t have that version of Node installed.

(The current behaviour appears to be to not do anything.)

That other nvm script switches automatically which worked well for me in the past.

(I only just noticed as my deployment failed complaining of a non-existent Nexe runtime as I was trying to deploy with 14.x while the project requires 12.x)

jorgebucaran commented 3 years ago

This issue was just an RFC. I didn't add any automatic version switching functionality. nvm-sh/nvm has none either.

That other nvm script switches automatically which worked well for me in the past.

I don't understand what you are describing. If you'd like to report a bug, create a new issue.

aral commented 3 years ago

@jorgebucaran OK, odd, I’ll test with the other nvm on a different machine when I get a chance and confirm. I’m sure it switched automatically but maybe I was confused. Will open an issue if not.

thernstig commented 3 years ago

@aral you might be using "that other nvm" together with e.g. direnv or similar tools, which does the version switching for you.

jorgebucaran commented 3 years ago

Yeah, but remember nvm-sh/nvm does not version-switch automatically on directory change, which is what this RFC was originally about.

ljharb commented 3 years ago

@aral nvm definitely does not auto switch by default, as that is a highly intrusive feature. You can use other tools, or a custom cd wrapper (code is on nvm’s readme) to opt in to that behavior.

mrcoles commented 1 year ago

…or a custom cd wrapper (code is on nvm’s readme) to opt in to that behavior.

For anyone who was curious, here’s where it is on their readme: https://github.com/nvm-sh/nvm#fish

daveyarwood commented 10 months ago

I also have a similar memory of nvm automatically switching to the Node version specified in .node-version or .nvmrc when I enter that directory. It was really nice not having to remember to run a command (even a simple one like nvm use) every time I open a new terminal in that project.

If nvm.fish could support this behavior, I think that would be really great, although I understand the complexity, having read the previous comments.

One idea I'll toss into the hat: I think it would be valuable just to do a simple check to see if the current directory contains a .node-version or .nvmrc file, and switch automatically if it does. @jorgebucaran said above that this is "not good enough" because ideally, it would need to look at the directories above, too, but I disagree about that being a hard requirement. 99% of the time, when I'm developing in a project, I stay in the top-level directory. I hardly ever have a use case for entering a sub-directory. Even if I did have such a use case, and I really, really wanted nvm to automatically switch to the Node version specified in the top-level .nvmrc, I could always work around that by making a symlink in the subdirectory to the top-level file. But that would be a rare exception.

I think that if nvm.fish simply looked for the file in the current directory and didn't bother to look at the directories above, that alone would be a big improvement. Just my 2¢ 🙂

daveyarwood commented 10 months ago

In case this is helpful for others, here is my hacky workaround in my fish config to automatically use Node 18 for a particular project:

function use-the-right-node-version --on-variable PWD
  if test $PWD = "$HOME/code/the-project"
    nvm use 18
  end
end

use-the-right-node-version

This simply checks for a hard-coded directory name, but if you wanted to, you could adjust the code to check for the existence of a .node-version or .nvmrc file at the top level and run nvm use if one exists.

But I still think it would be nice if nvm.fish did this for us 🙂

jorgebucaran commented 10 months ago

I also stay in the top-level directory most of the time. Your request isn't unreasonable at all, but in the meantime, you might want to check out this issue. It's quite easy to implement support for it in your config.fish if you really need it.

EDIT: I see you've already figured it out! 😉

daveyarwood commented 10 months ago

Thinking about it more: I like the idea of not automatically doing this for all users, as some users could find it intrusive, like the tool is doing too much. And it isn't hard to opt in in your user config, once you understand that Fish has an --on-variable feature that runs a function automatically when a variable changes (this was news to me!).

Maybe the missing piece here is to make it "official" in the README by explaining the philosophy of not switching versions automatically out of the box, and include a working fish config snippet for the users who want to opt into that behavior.

danielcompton commented 4 months ago

Maybe the missing piece here is to make it "official" in the README by explaining the philosophy of not switching versions automatically out of the box, and include a working fish config snippet for the users who want to opt into that behavior.

Yep, I think this would be good. My expectation was that this would happen automatically and I was quite confused reading the docs to not see a description of automatically loading when switching directories.

jorgebucaran commented 4 months ago

I believe I addressed that here, or at least I tried.

So, what do you think the issue is? Is it not visible enough, or is it unclear? While I understand you might have thought it worked differently, that may not be a totally common perception. But, I am open to improving the docs where possible. It's just that I feel it would be overkill to create a specific section just for this since it was never a feature in the first place and because of what I just said.

daveyarwood commented 4 months ago

I think anyone who has used the non-Fish version of nvm will expect nvm.fish to behave similarly, i.e. automatically switch to the Node version specified in .node-version or .nvmrc anytime you enter a directory that has one of those files.

Based on the discussion above, I think it's totally reasonable for nvm.fish not to work that way, but I think it would be good to add an "Automatic version switching" section to the README to make it absolutely clear that nvm.fish does not do this for you automatically, but that the behavior can be easily achieved by making a small adjustment to your fish.config.

jorgebucaran commented 4 months ago

But does nvm.sh automatically switch Node versions?

daveyarwood commented 4 months ago

Yes, I remember that it did, back when I used it.

On Thu, May 2, 2024, 5:22 PM Jorge Bucaran @.***> wrote:

But does nvm.sh automatically switch Node versions?

— Reply to this email directly, view it on GitHub https://github.com/jorgebucaran/nvm.fish/issues/132#issuecomment-2091683641, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFOQFY765UUNGW4BT7WVS3ZAKVBFAVCNFSM4VXOPWGKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMBZGE3DQMZWGQYQ . You are receiving this because you commented.Message ID: <jorgebucaran/nvm .@.***>

jorgebucaran commented 4 months ago

Okay. I might've missed a big detail. Would've assumed that they do not switch versions, at least not by default. I think we might need to mention this more prominently then.

ljharb commented 4 months ago

@daveyarwood no, it does not and 1000% never has. You are mistaken.

actual nvm has examples in its readme of how you can manually configure your system to do that, or how to use a zsh plugin to do it, but by default it would be incredibly unsafe and intrusive to switch versions automatically.

jorgebucaran commented 4 months ago

Ohhh, glad you explained that, it makes a lot more sense now, thanks!