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

NVM.fish — 2.0 🎏 #123

Merged
merged 34 commits into from
Dec 5, 2020
Merged

NVM.fish — 2.0 🎏 #123

merged 34 commits into from
Dec 5, 2020

Conversation

jorgebucaran
Copy link
Owner

@jorgebucaran jorgebucaran commented Nov 25, 2020

NVM.fish completely rewritten using Fish 3.0, introducing a new CLI. Not a new CLI you need to learn. One that is familiar to you, if you've used the original NVM project, but without the cruft. Implements #122.

The usual suspects are included: install, use, remove uninstall, list, list-remote, and current.

NVM 2.0 presents a more consistent command-line interface (#81, #111), better XDG support (#108), and leverages modern Fish idioms for improved performance. Also fixed (#101, #112), so that switching versions only lasts for the lifetime of the current shell. Want to change that? There's a way too.

nvm

@jorgebucaran jorgebucaran changed the title 2.0 NVM.fish — 2.0 🎏 Nov 25, 2020
@jorgebucaran jorgebucaran mentioned this pull request Nov 25, 2020
Copy link

@thernstig thernstig left a comment

Choose a reason for hiding this comment

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

I started from a validation perspective i.e. only commenting on the usage from the README.md perspective. I'll see if I have time to review the code later.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
- Use uninstall instead of remove
- Document nvm_mirror and nvm_default_version vars in help
README.md Show resolved Hide resolved
functions/nvm.fish Outdated Show resolved Hide resolved
@thernstig
Copy link

I am happy with the commit 🥇 💯 👍

- Fix bug in list-remote showing no output
- Improve node/npm version info shown when installing/using
@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 27, 2020

I think we're ready here. I won't be adding tests until Fishtape 3.0 is done. I'll probably land this tomorrow after going over the code one more time. If you want to try it out in the meantime, use:

fisher install jorgebucaran/nvm.fish@2.0

@thernstig
Copy link

I tested the new versions and found something. Is it expected? Without relaunching the shell, I tried e.g. nvm install <tab> and got:

nvm install <W> fish: An error occurred while redirecting file '/home/userA/.local/share/nvm/.index'
open: No such file or directory
<W> fish: An error occurred while redirecting file '/home/userA/.local/share/nvm/.index'
open: No such file or directory

Even if I specify a version manually and install I get another error. Is that expected? Relaunching the shell fixes it. I will try some functionality now when I have relaunched it.

@jorgebucaran
Copy link
Owner Author

I thought I had fixed that bug in da2cf70. Are you able to reproduce it? Thanks.

@thernstig
Copy link

thernstig commented Nov 29, 2020

A few things noticed.

  1. Sometimes I get this, not sure why:
nvm install 12.18.4
curl: (23) Failed writing body (346 != 4220)
Installing Node v12.18.4 lts/erbium
Fetching https://nodejs.org/dist/v12.18.4/node-v12.18.4-linux-x64.tar.gz
Now using Node v12.18.4 (npm 6.14.6) ~/.local/share/nvm/v12.18.4/bin/node
  1. If I install a version in one shell, and then open another, then I can properly do nvm list and it will show the installed version. But if I do nvm uninstall <version> it uninstalls it but exits with error code 1.
~> nvm list
   v12.18.4 lts/erbium
~> nvm current
~ [1]> nvm uninstall v12.18.4

~ [1]> nvm list
~ [1]>

This might actually also be the case where 1) is seen if I directly do a nvm install <version> after that. It installs it still though.

  1. direnv (https://github.com/direnv/direnv/releases) does not work perfectly anymore with the layout "use node" but that is something I will write an issue on the that repo once this has been released.

@thernstig
Copy link

I thought I had fixed that bug in da2cf70. Are you able to reproduce it? Thanks.

Was that fix included in jorgebucaran/nvm.fish@2.0 which was what I installed and tested. I just tested it 30 mins ago and got that failure.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Nov 29, 2020

Was that fix included in jorgebucaran/nvm.fish@2.0 which was what I installed and tested.

Yes, everything I push to this branch is what you get when you install @2.0. I just fixed a few things. Please, remove the plugin and install it again. Normally you'd just fisher update ..., but this is a special case because NVM.fish is not released yet.

direnv (https://github.com/direnv/direnv/releases) does not work perfectly anymore with the layout "use node" but that is something I will write an issue on that repo once this has been released.

It shouldn't be too hard to support nvm install/use node, but I'd much prefer if I didn't have to.

curl: (23) Failed writing body (346 != 4220)

I'm not sure what's happening. From the two curl uses we have, that should be here:

if not command curl --progress-bar --location --show-error $url \
| command tar --extract --gzip --directory $nvm_data/$ver 2>/dev/null
command rm -rf $nvm_data/$ver
echo -e "\033[F\33[2K\x1b[0mnvm: Invalid mirror or host unavailable: \"$url\"" >&2
return 1
end

I also found this on SO, but we're piping the output to tar, which I'd assume always reads the whole page. What else could it be? I noticed you're on Linux.

Could this be OS-specific? Do you happen to have a .curlrc file? What does it contain?

@thernstig
Copy link

I have no .curlrc file. I on Ubuntu 20.04. It did not happen every time, but sometimes. I agree it is quite strange. I will see if I can re-replicate this all soon, I unfortunately got a high-priority case to work with on my day-to-day job 😞

@jorgebucaran
Copy link
Owner Author

Did direnv put it there?

@jorgebucaran
Copy link
Owner Author

nvm puts it there, of course, when you install a version, and removes it when you uninstall it or use a different version (or switch to system if there's one). So, this could be a bug, but to be honest, I still don't understand what the issue you're having is.

Please provide me a longer dump of your terminal usage where the issue is clear.

@thernstig
Copy link

thernstig commented Dec 3, 2020

I was about to edit my reply. Yes, it sets it via the two env vars:

set -gx NODE_VERSIONS ~/.local/share/nvm
set -gx NODE_VERSION_PREFIX 'v'

See https://github.com/direnv/direnv/blob/f5b1566ab3f89c7e912ce0d1eb82ad8814247d30/stdlib.sh#L1068 calling https://github.com/direnv/direnv/blob/f5b1566ab3f89c7e912ce0d1eb82ad8814247d30/stdlib.sh#L625

@thernstig
Copy link

Still get it: curl: (23) Failed writing body (214 != 1891)

@jorgebucaran
Copy link
Owner Author

In that case, you did set it, albeit indirectly.

Okay, we can re-implement _nvm_current to play well with direnv.

Still get it: curl: (23) Failed writing body (214 != 1891)

Hmm.

@jorgebucaran
Copy link
Owner Author

@thernstig Last attempt using curl's --no-buffer flag. If this doesn't do it, I'll have to disable the progress bar, but that would be a shame. Or avoid the pipeline and redirect to a file, which wouldn't be so elegant. This is why we can't have nice things!

Please try again. 🙏

@thernstig
Copy link

In that case, you did set it, albeit indirectly.

Hmm.

Yes, to be fair, I did say I was about to edit my reply 😛

Please try again.

Maybe give wget a go? wget --show-progress --qO

I'd just redirect to a file to be done with it to keep the progress bar.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Dec 4, 2020

@thernstig Good news: the curl error should be fixed! 🙏

About direnv

I realized that letting direnv get away with just altering the PATH would miss the point of using nvm use, which is more than just switching versions. You'd lose the benefit of version validation and support for nvm_default_version. So, I decided to stick with nvm_current_version. Can direnv nvm use VERSION instead of altering PATH as you suggested?

For the record, this was my alternative _nvm_current implementation, just to show it's doable:

function _nvm_current
    set --local node (command --search node) || return
    string match --regex "^$nvm_data/v\d+\.\d+\.\d+" "" $node \
        | string replace --regex $nvm_data/ "" || echo system
end

@thernstig
Copy link

thernstig commented Dec 5, 2020

@thernstig Good news: the curl error should be fixed! 🙏

It is indeed fixed. You're awesome, great work! What was the exact problem?

About direnv

Can direnv nvm use VERSION instead of altering PATH as you suggested?

Yes, I think that might be more efficient and also less code for direnv. I'll write an issue once NVM 2.0 is released. I assume the only thing to figure out there in that project is to check if nvm.fish or nvm-sh is used and do an elif branch to instead just call nvm use when nvm.fish is used.

@jorgebucaran
Copy link
Owner Author

@thernstig Wouldn't it be better to use nvm use everywhere? I suppose the original nvm's nvm use does more than just juggling with the PATH, but I could be wrong. If and when you figure this out, I suggest using nvm use VERSION >/dev/null, since you might not care about the printout to stdout every time you open a new shell, but it's up to you.

@thernstig
Copy link

thernstig commented Dec 5, 2020

direnv handles a lot more than just nvm for me, and we use it as a shared tool to setup config for one project and when I juggle between project dirs I use it to not have to do any "use nvm" or other tools to setup the env for each project. That extra nvm use is a big annoyance stepping in/out of directories. This is what direnv solves so nicely.

Anyhow if we go back to this PR, it gets 5 thumbs up from me.

@jorgebucaran
Copy link
Owner Author

jorgebucaran commented Dec 5, 2020

@thernstig Sorry, I was not referring to direnv. Let me clarify.

I was trying to say: wouldn't it better if direnv used nvm use to modify the PATH for both nvm.fish (like we already agreed to) and nvm-sh/nvm, because I suppose the original nvm's use command does more than just modify your PATH, e.g., translating aliases like "carbon" into v8.17.0, and so on.

That extra nvm use is a big annoyance stepping in/out of directories. This is what direnv solves so nicely.

Totally. 🙆‍♂️

@jorgebucaran jorgebucaran merged this pull request into main Dec 5, 2020
@jorgebucaran jorgebucaran deleted the 2.0 branch December 5, 2020 18:08
@jorgebucaran jorgebucaran linked an issue Dec 5, 2020 that may be closed by this pull request
@thernstig
Copy link

Ah, yes, great idea. I will see if I can do a nice write-up of an issue for direnv later.

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.

Fish 3.0
3 participants