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

Auto-expand tilde (home directory) for universal variable `nvm_data` #190

Closed thernstig closed 1 year ago

thernstig commented 2 years ago

This is the same issue as fixed here https://github.com/jorgebucaran/fisher/issues/611

Currently nvm_data is set to something like nvm_data:/home/userA/\x2elocal/share/nvm which makes it harder to sync the fish_variables file. This could be solved the same way as was done for fisher, which would it possible/easier to sync the dotfiles.

(Since you are the author for both @jorgebucaran you know what this is about, hence why I am not more specific)

jorgebucaran commented 2 years ago

Or perhaps just change the variable scope to global? 🤓

thernstig commented 2 years ago

If that works ok, sure :)

debashisbiswas commented 1 year ago

Following the above recommendation, I changed nvm_data from universal to global, moving it out of fish_variables and into conf.d/nvm.fish (not config.fish, which happens after the default version is loaded), which worked for my use case.

Is this something you'd be interested in this project doing? If so, I wouldn't mind making the PR.

jorgebucaran commented 1 year ago

nvm_data is universal because https://github.com/jorgebucaran/nvm.fish/commit/2f2af433f75d1684f2c22c6ab58cc04e4176fdfc. If we can find a way to change it back to global without creating new issues, that'd be great.

rhenning commented 1 year ago

👋🏼 just stumbled across this while syncing config with chezmoi across a couple of machines, which inadvertently picked up the statically-rendered interpolation of nvm_data in fish_variables. i began to work around it with a template but then started to wonder how it got set... glad to see others are thinking about it too.

glad to help PR, test, buy coffee, etc. 😆

p.s. many thanks for your contributions to the community, @jorgebucaran. awesome work!

jorgebucaran commented 1 year ago

We could just revert the scope of nvm_data as per @debashisbiswas' suggestion.

See the commit above to see why we changed the scope to universal. Thank you, @rhenning! 😁

lorthirk commented 1 year ago

👋🏼 just stumbled across this while syncing config with chezmoi across a couple of machines

The world is such a small place... I am in the very same situation! I've been watching this issue for a long time, and I am also available for anything!

debashisbiswas commented 1 year ago

nvm_data is universal because 2f2af43. If we can find a way to change it back to global without creating new issues, that'd be great.

I read through the issues that this commit was meant to fix. To provide context for this discussion: the reason it was made universal is to avoid requiring the user to restart their terminal after installing, and to avoid issues in which their config was set up to use nvm before nvm.fish was run.

lorthirk commented 1 year ago

I would love to think of a way to fix this, but I'm afraid that the context @debashisbiswas provided is not enough for me to fully understand ths issue 😕 any other hint would be greatly appreciated

debashisbiswas commented 1 year ago

@lorthirk It's been a few months, so I've forgotten the specifics, but I can try to give you a bit more context. Looking at https://github.com/jorgebucaran/nvm.fish/commit/2f2af433f75d1684f2c22c6ab58cc04e4176fdfc and the related issues, there seem to be a few reasons why the variable was made universal. The issues in question are #130, #139, and #140.

Universal variables are applied immediately, and available to all sessions. On the other hand, global variables are per-session, and must be set before they are used within the session. Previously, the global variables were set in conf.d/nvm.fish, which runs when the session starts. Right after a fresh install, this file hasn't been run yet, so trying to use nvm immediately after installing resulted in errors. This means that users needed to start a new session after install. This caused some confusion for users, so universal variables were used instead. I believe this is what happened in #130 and #140. It should be possible to achieve the same behavior with global variables if they're managed correctly.

Also, config files in fish are loaded on startup in a specific order. If the user's config is set up to use nvm before conf.d/nvm.fish is run (for example, by using nvm install in conf.d/aaa.fish, which would run before), there will be issues with variables being uninitialized, which results in the same error as the previous paragraph. This is what happened in #139, and universal variables solve this as well. I am not sure that there's a way around this with global variables, but this case may not be necessary for this plugin to handle, as this is more specifically related to how a user's fish config is set up.

lorthirk commented 1 year ago

Thanks @debashisbiswas. Now that you explained it in such an easy to understand way, I'm not so sure any fix we could come up with would be better than "hackish". I have a naive question: would be that bad to add a warning message when installing nvm.fish that warns about such issues? Honestly, restarting a session is not that bad once you know you have to do, and in general I wouldn't let this drive the architecture of an application. As per the other issue, as you said, this really sounds like something a user should be warned of and arrange his/her custom configuration accordingly.

What do you guys think?

jorgebucaran commented 1 year ago

Fixed. Thanks, everyone! 🙌