wbyoung / avn

Automatic Version Switching for Node
MIT License
1.14k stars 54 forks source link

avn fails when .nvmrc contains additional lines #55

Open lachlanhunt opened 7 years ago

lachlanhunt commented 7 years ago

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

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

Yes.

ljharb commented 7 years ago

@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 7 years ago

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.

wbyoung commented 7 years ago

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:

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

ljharb commented 7 years ago

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

wbyoung commented 7 years ago

@ljharb does that traverse from CWD?

ljharb commented 7 years ago

yes

wbyoung commented 7 years ago

@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!