Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

avn fails when .nvmrc contains additional lines #55

Open
1 task done
lachlanhunt opened this issue Apr 27, 2017 · 7 comments
Open
1 task done

avn fails when .nvmrc contains additional lines #55

lachlanhunt opened this issue Apr 27, 2017 · 7 comments

Comments

@lachlanhunt
Copy link

lachlanhunt commented Apr 27, 2017

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
Copy link

ljharb commented Apr 27, 2017

@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
Copy link
Author

lachlanhunt commented Apr 27, 2017

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
Copy link
Owner

wbyoung commented Apr 27, 2017

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
Copy link

ljharb commented Apr 27, 2017

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

@wbyoung
Copy link
Owner

wbyoung commented Apr 27, 2017

@ljharb does that traverse from CWD?

@ljharb
Copy link

ljharb commented Apr 27, 2017

yes

@wbyoung
Copy link
Owner

wbyoung commented Apr 28, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants