-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Respect previously nvm-loaded node version when re-sourcing #1315
Conversation
Just thought of something: if there's no |
Yes, it returns "none". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be sourcing nvm.sh
more than once that wouldn't be checking for nvm
functions already existing?
nvm.sh
Outdated
nvm use --silent "$VERSION" >/dev/null | ||
elif nvm_rc_version >/dev/null 2>&1; then | ||
nvm use --silent >/dev/null | ||
if [ "_$NVM_CURRENT" = '_system' ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current behavior is that whenever I re-source nvm.sh
, I get the auto-used version. With this change, that won't be the case.
(also, yes, this needs to handle "none")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, that's the idea. The fact that NVM assumes, whenever it's sourced, that the environment is uninitialized is a hindrance to me.
(I've pushed a fix to handle none
.)
My particular use case is when a shell script that I'm also using Without this fix, that means as soon as I run a global npm cli executable (which are also lazy-loaded by With this fix, the loaded version of node persists. |
I'll leave this open while I think more about it, but as discussed in #1308, there's a few reasons I'm hesitant. "lazy loading" is not a supported use case for |
I appreciate that lazy loading isn't officially supported. But without it, NVM is simply too much overhead to be worth it for me. Using lazy loading for NVM changed my shell startup time on my laptop from 4+ seconds to <0.5 seconds. I'm a big tmux user and I'm opening up shells all. the. time. so that startup time has a big impact in my ability to remain in a "flow" state when developing. Of course the functions have to be loaded for NVM to do its work. Respecting a previously loaded version of node (by another instance of NVM, even if that instance is no longer present in the current shell) opens up possibilities for NVM to be used in more non-interactive ways, not just by direnv, but also by deployment tools & workflows, and many other as-yet-unimagined uses. Neither #1316 nor this pull request break anything (as far as the tests are able to indicate), so there should be no impact on current users. I do realize that both these pull requests are decisions about the future of NVM, how it might be used, where it fits in. I appreciate your considered attention to these requests. |
In my use case, as soon as any |
@ljharb I've been struggling with this build issue for ages. If I run the code inside the test manually (after first running Would you mind taking a look to see if you can determine why this test is failing? |
NVM_LS_CURRENT="$(nvm ls current | strip_colors | \grep -o v0.10.1)" | ||
[ "_$NVM_LS_CURRENT" = '_v0.10.1' ] || die "'nvm ls current' did not return '-> v0.10.1', got '$NVM_LS_CURRENT'" | ||
[ "_$NVM_LS_CURRENT" = '_v0.10.1' ] || die "'nvm ls current' did not return '-> v0.10.1', got '$NVM_LS_CURRENT_NOT_GREPPED'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it might be helpful here to do nvm ls ; die
and see what shows up
@ljharb I managed to figure out how to run the tests locally ( Basically, it seems that on the TravisCI VM, Since this commit specifically makes My solution is to fix the test by unloading Rebased on master. 1: Is this desired behaviour? If someone has an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zeorin thanks for the explanation; I think I understand.
nvm.sh
Outdated
elif nvm_rc_version >/dev/null 2>&1; then | ||
nvm use --silent >/dev/null | ||
if [ "_$NVM_CURRENT" = '_none' ] || \ | ||
[ "_$NVM_CURRENT" = '_system' ]; then |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Sometimes
nvm.sh
may be sourced more than once (e.g. when usingdirenv
andnvm-zsh
's lazy-loading feature). If nvm has already loaded a version of node that is notsystem
, don't change that version.In practice this means that if the currently active version of node is not
system
, nvm.sh will not change the version of node that is active.If the version is
system
, it will still activatedefault
.