Skip to content

Unset NVM_BIN when is not necessary #1033

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 11, 2016
Merged

Unset NVM_BIN when is not necessary #1033

merged 1 commit into from
Mar 11, 2016

Conversation

robsonpeixoto
Copy link
Contributor

I would like to know if my current shell are using a node configured by the nvm.
But the are only 3 cheap ways to know

  • check if there are the nvm directory on PATH
  • check the version using the NVM_BIN or NVM_PATH

Using the NVM_BIN is very simple. Using this simple function:

function __node_ps1() {
  if [[ ! -z "$NVM_BIN" ]]; then
    local array=(${NVM_BIN//// })
    local version=${array[-2]}
    echo " js(${version##*v})"
  fi
}

But after a nvm deactivate or nvm use system the nvm keep these environment variables.

ljharb
ljharb reviewed Mar 10, 2016
View reviewed changes
nvm.sh
@@ -1913,11 +1914,13 @@ nvm() {
if [ $NVM_USE_SILENT -ne 1 ]; then
echo "Now using system version of node: $(node -v 2>/dev/null)$(nvm_print_npm_version)"
fi
unset NVM_BIN NVM_PATH
Copy link
Member

Choose a reason for hiding this comment

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

this line shouldn't be needed, since you're doing it in nvm deactivate already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 0227822

@ljharb
Copy link
Member

ljharb commented Mar 10, 2016

@robsonpeixoto it's much simpler than that. type nvm >/dev/null 2>&1 && nvm current will exit 0 and print out the current nvm-managed node version if nvm is available. I use that in my own prompt and it's very fast.

@ljharb ljharb added the needs followup We need some info or action from whoever filed this issue/PR. label Mar 10, 2016
ljharb
ljharb reviewed Mar 10, 2016
View reviewed changes
nvm.sh
@@ -1870,6 +1870,7 @@ nvm() {
echo "$NVM_DIR/*/lib/node_modules removed from \$NODE_PATH"
fi
fi
unset NVM_BIN NVM_PATH
Copy link
Member

Choose a reason for hiding this comment

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

should this have -f so that it will try to unset it even if it happens not to be set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-f is useful to remove function definition.
And I do not see any problem if it unset a variable that no dot exists.

~> export OI=123
~> echo $?
0
~> unset OI
~> echo $?
0
~> unset OI
~> echo $?
0

Copy link
Member

Choose a reason for hiding this comment

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

k thanks, just confirming :-)

@ljharb
Copy link
Member

ljharb commented Mar 10, 2016

Assuming tests pass, I think this looks great! Would you mind rebasing it down to a single commit?

@ljharb ljharb added bugs Oh no, something's broken :-( needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. and removed needs followup We need some info or action from whoever filed this issue/PR. labels Mar 10, 2016
@robsonpeixoto
Copy link
Contributor Author

I can, @ljharb. Before or after the tests finish ?

@ljharb
Copy link
Member

ljharb commented Mar 10, 2016

Either way, but I suppose we might as well wait til they pass to make sure there's no further tweaks needed. Thanks!

@robsonpeixoto
Copy link
Contributor Author

Done @ljharb.

@ljharb
Copy link
Member

ljharb commented Mar 11, 2016

@robsonpeixoto awesome! if you'd mind one more rebase that'd be awesome, otherwise i'll merge this in later today :-D

@robsonpeixoto
Copy link
Contributor Author

A rebase with the master done, @ljharb

@ljharb ljharb removed the needs rebase Please rebase your branch onto latest master! This removes merge commits, & keeps the git log clean. label Mar 11, 2016
@ljharb
Copy link
Member

ljharb commented Mar 11, 2016

Thanks!

@ljharb ljharb merged commit 8fb4ee0 into nvm-sh:master Mar 11, 2016
@robsonpeixoto robsonpeixoto deleted the unset-unnecessary-envvar branch March 11, 2016 19:24
sandinmyjoints added a commit to sandinmyjoints/nvm that referenced this pull request Jun 24, 2016
@sandinmyjoints
Merge tag 'v0.31.1'
faefb90 
v0.31.1 Changelog

New Stuff:
 - `nvm uninstall`: Check installation dir permissions before uninstalling; display "fix" commands (nvm-sh#847)
 - `nvm alias`: colorize output to match `nvm ls`
 - `nvm alias`: colorize output when creating aliases
 - `nvm ls`/`nvm alias`/`nvm ls-remote`: only colorize when colors are supported

Fixes:
 - don’t use bash `==` in conditionals
 - `nvm run`: pass through `--silent` on bare `nvm run`
 - `nvm exec`: show “io.js” for io.js versions
 - `set -e`: ensure `nvm_version` returning 3, and `nvm_alias` returning 2, doesn’t terminate the process
 - `nvm alias`: explicitly forbid user aliases in subdirs
 - `read` exits 1 when `.nvmrc` lacks a trailing newline; avoid that
 - `set -x`: avoid an unbound variable
 - `deactivate`: unset `$NVM_BIN` and `$NVM_PATH` (nvm-sh#1033)

Performance:
 - `nvm alias`: slightly speed up alias resolution
 - Use `awk` to improve version comparison performance

Robustness:
 - add a missing `command` to a `sed` call

Misc:
 - Various README tweaks
 - Various testing improvements
 - Prefer `nvm --help` over `nvm help`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bugs Oh no, something's broken :-(
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robsonpeixoto @ljharb