jorgebucaran / nvm.fish

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

`nvm use` should always activate given version regardless of `nvm_current_version` #222

Open dangh opened 4 months ago

dangh commented 4 months ago

Hello,

This is my setup.

The culprit is the check here:

https://github.com/jorgebucaran/nvm.fish/blob/c69e5d1017b21bcfca8f42c93c7e89fff6141a8a/functions/nvm.fish#L150-L153

I don't know the reason for this, but I would expected nvm use to always active the given version.

What do you think?

jorgebucaran commented 4 months ago

nvm_current_version is internal to nvm.fish and isn't meant for user modification. Can you describe the actual problem you had? I'm not sure what you're attempting.

dangh commented 4 months ago

The script above is just for debugging the issue. I'm not using nvm_current_version directly. Only nvm use.

Let me try again.

  1. In my interactive shell, I ran nvm use to read from .nvmrc. nvm_current_version is resolved and exported. Let say it's 16
  2. I run my build script. Inside my build script, I ensure the node version by nvm use again. Ideally, it should read .nvmrc and update PATH accordingly. But because nvm_current_version is imported from interactive shell above (both are 16), nvm choose to skip it and does not updating PATH.
  3. The problem arise when I have a pinned node 20 in my config (archived using fish_user_paths and is NOT managed by nvm). Because nvm use doesn't updating PATH in step 2, my script failed because it run on a different version of node.

My use case is rare as node should always be managed by nvm. But it's unavoidable for me. And I would say, nvm use should be deterministic and always updating the PATH accordingly.

jorgebucaran commented 4 months ago

What steps did you follow according to the nvm documentation, and what didn't work?

dangh commented 4 months ago

I only use nvm use to switch to version stated by .nvmrc.

You can do these steps to reproduce it.

# assuming you has node v20.11.1 in your system
fish_add_path ~/.local/share/nvm/v20.11.1/bin

# inside test dir, we need node 16
echo 16 > .nvmrc

function my_script
    nvm use
    node -v # -> it's 16 as expected
    fish -c '
       nvm use
       node -v # -> expected 16 but its 20
    '
end

# run test script inside test dir
my_script
jorgebucaran commented 4 months ago

Why do you need to run nvm use twice?

dangh commented 4 months ago

The first is for my interactive use. The one inside the script is because I want to run it for multiple projects that use different node versions.

jorgebucaran commented 4 months ago

But do you need to start a new shell in your function? nvm is designed for interactive use first.

dangh commented 4 months ago

The first nvm use should be outside my_script. Sorry my bad example cause a confusion.

I spin a new shell in the function because there are some other stuff that will affect my interactive shell that I don't want to.

jorgebucaran commented 4 months ago

Not sure why your setup is so restrictive, but it seems like you might be using the wrong tool for the job. Regardless, I'm not sure about the ideal nvm use behavior in a subshell. Wonder what nvm.sh does in this context.

dangh commented 4 months ago

I never use nvm.sh before so I'm not sure. nvm.fish is managed node for me so I want to rely on it as much as I can.

I have some questions.

jorgebucaran commented 4 months ago

Great questions. Perhaps exporting isn't necessary, but I am sure I had a good reason for it when I originally wrote the code. I'll need to take some time to revisit this. Can't say exactly when, but for now, consider removing --export from:

https://github.com/jorgebucaran/nvm.fish/blob/c69e5d1017b21bcfca8f42c93c7e89fff6141a8a/functions/_nvm_version_activate.fish#L1-L4

dangh commented 4 months ago

Thanks you. For now I'll fork nvm.fish and unexporting it. Also thanks a ton for making fisher. It makes installing packages super convenient.