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

Allow setting nvm's custom install directory #231

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

davychhouk
Copy link

@davychhouk davychhouk commented Oct 26, 2024

Description:

Allow setting nvm's custom install directory.

If $nvm_data is set, it would use this path as the install directory.
Otherwise default to $XDG_DATA_HOME.

Changes:

  • Only set $nvm_data to $XDG_DATA_HOME if $nvm_data is not set.
  • Update README.md detailing how to set $nvm_data.
  • Update help command to show $nvm_data variable.

Ref: #146

@jorgebucaran
Copy link
Owner

Thank you! I'll take a little time to think it over and decide if I want to go for it or not.

@jorgebucaran jorgebucaran added the enhancement New feature or fix label Oct 27, 2024
@smorimoto
Copy link

I really want this!

@smorimoto
Copy link

You have to add that variable here:

echo "Variables:"
echo " nvm_arch Override architecture, e.g. x64-musl"
echo " nvm_mirror Use a mirror for downloading Node binaries"
echo " nvm_default_version Set the default version for new shells"
echo " nvm_default_packages Install a list of packages every time a Node version is installed"

@davychhouk
Copy link
Author

I've updated the help desc.

@jorgebucaran
Copy link
Owner

How about calling it nvm_data instead of nvm_dir?

@davychhouk
Copy link
Author

This makes sense, I've refactored the changes.
Please have a look. Thanks!

@smorimoto
Copy link

The latest branch is unable to resolve the version with the default node version variable.

@davychhouk
Copy link
Author

davychhouk commented Dec 14, 2024

It should be good now.
If there is a better way to do this, please let me know.
Thanks!

conf.d/nvm.fish Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
conf.d/nvm.fish Outdated Show resolved Hide resolved
Copy link
Owner

@jorgebucaran jorgebucaran left a comment

Choose a reason for hiding this comment

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

LGTM :)

@smorimoto
Copy link

🎉

@smorimoto
Copy link

Oops, it still does not seem to resolve the default version 🤷‍♂️

@davychhouk
Copy link
Author

Ohh really?
My quick test, it was working. Let me double-check again then.

@jorgebucaran
Copy link
Owner

This PR enables a simpler way to customize the data directory. It does not affect the default version, or did I misunderstand you?

Set where nvm stores Node binaries and related data. Defaults to `$XDG_DATA_HOME/nvm` (~/.local/share/nvm) if unset.

```fish
set --global nvm_data ~/.nvm

Choose a reason for hiding this comment

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

If set globally, it will not work.

Suggested change
set --global nvm_data ~/.nvm
set --universal nvm_data ~/.nvm

Copy link
Owner

Choose a reason for hiding this comment

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

What will not work exactly?

Copy link
Owner

Choose a reason for hiding this comment

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

You need to put it in your config.fish. Unless we intended this feature to work interactively, which I did not assume.

Choose a reason for hiding this comment

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

It causes the following error:

nvm: Can't use Node "22", version must be installed first

Choose a reason for hiding this comment

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

Of course I added that to config.fish, but it did not work unless I set it as universal.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, thanks. I'll investigate then!

Choose a reason for hiding this comment

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

It may have been shadowed by setting the variable in different ways.

@smorimoto
Copy link

smorimoto commented Dec 17, 2024

I don't know why only global does not work with the default version, but this seems like the reason.

# Blow up everything
set --erase --universal --global nvm_data

# This does not work...
set --global nvm_data $HOME/.nvm

# Blow up again
set --erase --global nvm_data

# This works
set --universal nvm_data $HOME/.nvm

@smorimoto
Copy link

Anyway, just update the documentation and then we can go ahead!

@smorimoto
Copy link

Oops, wait, now it works. There is a possibility that I made a mistake in the somewhere.

@smorimoto
Copy link

You may also have to update the documentation to set nvm_default_version and nvm_default_packages globally, not universally.

@jorgebucaran
Copy link
Owner

We need one of those to be universal when setting the default version for new shells because your config.fish is sourced last.

@smorimoto
Copy link

I actually had to set both the default version and the data directory as universal.

@jorgebucaran
Copy link
Owner

I'll investigate.

@smorimoto
Copy link

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants