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 use of NVM_AUTO_USE alongside NVM_LAZY_LOAD on init #55

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

runofthemill
Copy link

Right now, NVM_AUTO_USE=true will not get applied on the initial call of nvm/npm/node/yarn, and only work on subsequent session directory changes. This PR makes NVM_AUTO_USE=true go into effect at the first invocation when NVM_LAZY_LOAD=true.

Note - I'm good enough at tinkering in shell scripts, but totally could be doing something dumb here! Thanks for taking a look and making a great plugin!

@amirdt22
Copy link

amirdt22 commented Jul 28, 2019

Thanks, I tried and failed to write an urchin test for this, but it's working for me locally!

@runofthemill
Copy link
Author

Whoops! I forgot about tests, didn't see the test failed until just now. Thanks @amirdt22 - I'll take a look at your PR :)

Add failing test for auto and lazy
@runofthemill
Copy link
Author

@amirdt22 @lukechilds any thoughts on why the tests are failing? Running the tests locally with urchin fails on NVM_LAZY_LOAD && NVM_NO_USE, but every other test passes.

@MuhmdRaouf
Copy link

@runofthemill, first of all, thank you a lot man for this PR
but I have a weird issue maybe it's the reason for the failing tests or something

my prompt shows the node version and strangely when I navigate to project has .nvmrc
it shows the message as it already loaded

but I had to run any command first so it runs properly and shows the real node version

snowflake in saleor on  master is 📦 v0.0.0 via ⬢ Found '/Users/snowflake/Playground/saleor/.nvmrc' with version <10.15.2>
Now using node v10.15.2 (npm v6.4.1)
v10.15.2 on 🐳 v19.03.8 via 🐍 system
[I] ➜ node -v
Found '/Users/snowflake/Playground/saleor/.nvmrc' with version <10.15.2>
Now using node v10.15.2 (npm v6.4.1)
v10.15.2

snowflake in saleor on  master is 📦 v0.0.0 via ⬢ v10.15.2 on 🐳 v19.03.8 via 🐍 system took 3s
[I] ➜

@runofthemill
Copy link
Author

@MuhmdRaouf you're talking about how it's showing the message inside your prompt? how is that section of your prompt defined?

@MuhmdRaouf
Copy link

@runofthemill I am using Zsh SpaceShip Theme
and it shows the current node version, whenever I move to the folder contain .nvmrc file instead of showing node version it shows,

Found '/Users/snowflake/Playground/saleor/.nvmrc' with version <10.15.2>
Now using node v10.15.2 (npm v6.4.1)
v10.15.2

instead of node version.

nvm will already be called by the line above (_zsh_nvm_load) so this a) takes longer and b) causes duplicated output in the terminal, e.g.:

Found '/Users/[...]/.nvmrc' with version <9.2.1>
Now using node v9.2.1 (npm v5.5.1)
Found '/Users/[...]/.nvmrc' with version <9.2.1>
Now using node v9.2.1 (npm v5.5.1)
@runofthemill
Copy link
Author

@MuhmdRaouf I just checked and looks like the theme would need to add some logic here:

https://github.com/denysdovhan/spaceship-prompt/blob/2796da9d5c4a6af7a8255315aa5eeae4aa929679/sections/node.zsh#L31-L33

In iTerm2 I use this to conditionally set the node version in the app status bar, you could probably use something similar in the theme:

function set_node_version() {
  if (type _zsh_nvm_nvm &>/dev/null); then
    echo $(nvm current)
  else
    echo "unset"
  fi
}

@runofthemill
Copy link
Author

@lukechilds I finally got tests to pass (yay escaping things!) - would love your thoughts on the PR whenever you get a chance!

@@ -102,6 +102,7 @@ _zsh_nvm_lazy_load() {
eval "$cmd(){
unset -f $cmds > /dev/null 2>&1
_zsh_nvm_load
[[ \"$NVM_AUTO_USE\" == true ]] && add-zsh-hook chpwd _zsh_nvm_auto_use
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to setup the chpwd hook here?

Shouldn't it already be set here?

https://github.com/lukechilds/zsh-nvm/pull/55/files#diff-dfc5680d74f94037e00d26cbeba9485eR212

Copy link
Owner

Choose a reason for hiding this comment

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

The tests all still pass if this line is removed:

https://travis-ci.org/github/lukechilds/zsh-nvm/jobs/687714916

30f8555

Choose a reason for hiding this comment

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

This is what you should do here:

[[ \"$NVM_AUTO_USE\" == true ]] && _zsh_nvm_auto_use

The hooks are already loaded, you just need to run _zsh_nvm_auto_use after loading nvm

@nicoladefranceschi
Copy link

This is what you should do here:

[[ "$NVM_AUTO_USE" == true ]] && _zsh_nvm_auto_use
The hooks are already loaded, you just need to run _zsh_nvm_auto_use after loading nvm

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.

5 participants