wbyoung/avn

avn fails when .nvmrc contains additional lines

lachlanhunt opened this issue · 7 comments

lachlanhunt commented

When .nvmrc contains more than 1 line, e.g. with comments, then avn fails.

This is a problem because a code base I have to work with at work has extended .nvmrc with some additional lines for specifying the yarn version, which is used by a different tool. These extra lines are ignored by nvm, but cause an error in avn.

The simplest fix would be to only read the first line of the file and ignore everything else that follows.

i.e. In lib/hooks.js, change this line:

.then(function(version) { this.version = version.trim(); })

to this:

.then(function(version) { this.version = version.split('\n')[0].trim(); })

Details

  • avn 0.2.3
  • node 6.9.4
  • nvm 0.33.1
  • bash 4.4.12

The output of __avn_debug in the directory with a .node-version file is:

avn could not activate node 6.9.4
# test
error: no plugin passed predicate
  avn-nvm: no version matching 6.9.4
# test
  avn-n: no version matching 6.9.4
# test

avn is loaded in my ~/.{bash|zsh}{_profile|rc} file with:

[[ -s "$HOME/.avn/bin/avn.sh" ]] && source "$HOME/.avn/bin/avn.sh" # load avn

nvm specific

  • As an nvm user I am confirming that I did not install with Homebrew

Yes.

ljharb commented

@lachlanhunt as the maintainer of nvm, I absolutely intend to break your use case in the future. Please do not use nvm's config file for anything that's not part of nvm.

However, this is still an issue with avn; avn should not be relying on the file only ever having one line in it.

lachlanhunt commented

I agree it shouldn't be abused for things unrelated to node versions. Unfortunately, that code is maintained by a different team and I'm not entirely sure why they're doing it. I'll follow up with them though to see if they'll agree to change it.

But at the very least, ignoring lines beginning with # and treating them as comments would help.

  • 👍1
wbyoung commented

I think this is something that will be somewhat difficult to fix "correctly".

Here are a few related issues that may further the discussion here:

Here are some options to consider:

  • Remove support for .nvmrc files since it has caused many issues.
  • Use an nvm call to read the correct info (or even find config files based on the current directory) and get back a version. Perhaps this sort of call already exists. If not, we'd need to first add something to nvm.

As I haven't been using nvm heavily, I'd very much appreciate any PRs related to this.

ljharb commented

nvm_rc_version will set the $NVM_RC_VERSION environment variable if an nvmrc file is found.

wbyoung commented

@ljharb does that traverse from CWD?

ljharb commented

yes

wbyoung commented

@ljharb thanks. So avn should be able to use that to read the file rather than making any assumptions. @lachlanhunt if you want to take a stab at that a PR would be more than welcome!

  • 👍1