nvm icon indicating copy to clipboard operation
nvm copied to clipboard

Add CLI option to set where to get the node version from, other than `/.nvmrc`

Open victormihalache opened this issue 2 years ago • 21 comments
trafficstars

In my quest to try and have a clean root for my project, I have moved most of my config files to a config folder. I was able to do so because I could specify to the various tools that their config file was located there via a CLI option like --config <path>.

nvm does not provide a way to set such path. It would be handy to be able to run something like nvm use --config config/nvm or nvm use -c config/nvm and read the given file like it would read .nvmrc.

To further expand, one could put a .nvmrc file in a folder—say config—such that when the -c option is used by just giving the path to a folder, like nvm use -c config, it would look for a .nvmrc in that folder. Or, even better in my opinion, have it search for more than just /.nvmrc (personal taste, ./config/nvm/config would be the best).

victormihalache avatar Nov 21 '23 14:11 victormihalache

As I'm still learning how the codebase works, and as the CLI flag option looks better (since deciding to use the ./config/nvm/.nvmrc is just as arbitrary as using ./.nvmrc), I won't add this to an eventual PR, but I'll share it here in case it helps anyone.

To set config/nvm/.nvmrc as another viable config file to have nvm look for one must change from L453 onwards to:

nvm_find_up() {
  local path_
  path_="${PWD}"
  while [ "${path_}" != "" ] && [ "${path_}" != '.' ] && [ ! -f "${path_}/${1-}" ] && [ ! -f "${path_}/config/nvm/${1-}" ]; do
    path_=${path_%/*}
  done
  nvm_echo "${path_}"
}


nvm_find_nvmrc() {
  local dir
  dir="$(nvm_find_up '.nvmrc')"
  if [ -e "${dir}/.nvmrc" ]; then
    nvm_echo "${dir}/.nvmrc"
  elif [ -e "${dir}/config/nvm/.nvmrc" ]; then
    nvm_echo "${dir}/config/nvm/.nvmrc"
  fi
}

victormihalache avatar Nov 21 '23 16:11 victormihalache

I personally think that's a fruitless quest; many tools aren't configurable in that way, and I'm not all that interested in making nvm more complex in pursuit of that goal.

ljharb avatar Nov 21 '23 20:11 ljharb

I do agree that many tools are not—but the most widely-used ones are. Look at typescript (the --project, -p flag), prettier, or eslint for example: they all allow you to specify a path to your config file.

It's not that difficult to add the flag, I've already opened a PR, look at #3235.

victormihalache avatar Nov 21 '23 20:11 victormihalache

By "complexity" i don't mean difficulty to add - flags are trivial - but more logic present in the code, more to maintain, more chance of bugs.

TS and prettier and eslint are very different kinds of tools with very large config files that often need to be managed by different teams than those using them, and that often need to compose with other config files. They're just not in the same arena as nvm's very simple line-based config file (that only currently supports a single value).

nvm commands are often run in succession; are you really going to want to append --config to every command? Note that if you make an alias for nvm, it will likely break things in lots of subtle ways. This can lead to lots of additional bug reports for me to spend time triaging, let alone the users who run into trouble but don't file a bug report at all.

ljharb avatar Nov 21 '23 20:11 ljharb

I personally do not find the added logic to add that much complexity, but if you think so then it's fine—you are the one mostly maintaining the project after all.

I do not fully agree with claiming that just because the config is small, then it's not worth having it modular in this way. It is a similar discussion to the XDG_CONFIG_HOME discussion, we should not force users to have a cluttered root or restrict them for no reason, especially when a solution is available.

Regarding the length of the command: yes it could be an annoyance, but...

The proposed solution is 100% backwards compatible with the current way of doing things: those who have an .nvmrc file can continue to use it, and those that want to make it can continue to do so.

The flag is not forced upon people, so if you don't need to tuck the config away in a separate place, one does not have to do so. But for those that need/want to do so fully acknowledge that they have to type in the flag every time they run the code (but one could also add a script to their package.json, or make a script to prep the environment for new people on the team)

victormihalache avatar Nov 21 '23 20:11 victormihalache

"cluttered" is an opinion, not a fact; having config files in the root means they're exactly where everyone expects to find them, precisely because neither XDG nor "config/" nor anything else is a universal, or even majority, setup.

I definitely agree it's backwards compatible, which is good and necessary.

What happens, though, when there's an .nvmrc and a config/.nvmrc (or whatever), and people get different results depending on whether they provide an option or not? I've seen this kind of confusion happen before with the exact tools you mentioned.

ljharb avatar Nov 23 '23 04:11 ljharb

In my opinion this is how it should be done:

  1. Follow the user's preference—if given
  2. Follow historic order

In this case it means that:

  1. If the user specified a config flag that should be respected, full stop.
  2. If no flag has been specified then the historic order is followed: 2.1. Use the .nvmrc at the root 2.2. Use the .nvmrc in the "new" location (that can be ./config/nvm/.nvmrc

This way we make sure that the user gets what they expect: those that want a custom config use that, those that do not care can place the config in either location, and those that do not want to change anything can keep doing things as things have always been done before.

victormihalache avatar Nov 24 '23 10:11 victormihalache

as a side note, nvm won’t ever have more than one local config file; a directory for them seems overkill.

By default i wouldn’t want any “new” location, either.

ljharb avatar Nov 24 '23 15:11 ljharb

Well than the case is even more simple: just look for the "usual" /.nvmrc by default—unless otherwise specified via a flag—and that's it. No new defaults, no new nothing.

Code-wise it's basically all done in the PR I made.

victormihalache avatar Nov 27 '23 13:11 victormihalache

I support an alternative, with no configuration, where nvm would also search for .node-version in all places that it searches for .nvmrc.

This would allow people to add this file to their repository without being opinionated with which Node versioning tool they are using.

fulldecent avatar Nov 27 '23 18:11 fulldecent

@fulldecent your proposal basically adds another possible name for a config file; one could argue that other programs could, instead, read the .nvmrc file themselves, or have a way to tell them which file to read—like with my proposal.

As things currently stand with my PR you can just run nvm use --config=.node-version

victormihalache avatar Nov 28 '23 13:11 victormihalache

Everyone, except NVM already supports the other file name

fulldecent avatar Nov 28 '23 13:11 fulldecent

Since I've never heard of a .node-version file—due to me always using nvm—, what other programs use it? I, for example, don't think volta support it (a quick rg on the repo for "version" didn't provide any meaningful results regarding fetching files)

victormihalache avatar Nov 28 '23 13:11 victormihalache

Please find extensive documentation and feature matrix here

https://github.com/shadowspawn/node-version-usage

fulldecent avatar Nov 30 '23 22:11 fulldecent

I could add initial support for the .node-version file by just reading it as nvm currently does for its .nvmrc, but I reckon that is out of scope for the current issue (I initially made it just for adding a flag to change the location to read a file from).

Thus far I'm waiting for @ljharb to approve the new flag without changing anything for those that don't use the flag.

victormihalache avatar Dec 01 '23 09:12 victormihalache

Supporting a different file format like node-version is off topic here; see #794.

@victormihalache i agree that just adding the flag with no other changes is technically trivial; I’m just not convinced it’s a valuable enough feature to add. In software, yes is permanent and no is temporary, so adding things must always be treated as more dangerous than declining a feature request.

I’m going to leave this open until I’ve decided one way or another, as closing it would mean we’d never add it.

ljharb avatar Dec 01 '23 15:12 ljharb

Hey guys I'm finally catching on a little may I have a study teacher to verbally give me a quick rundown. Lol not kidding. What do I fix first since I'm just now seeing these messages! I'm truly sorry if I've let anyone down

MGLPollockjr avatar Dec 14 '23 23:12 MGLPollockjr

Would there be any downside to allowing the user to chose the path they best prefer?

victormihalache avatar Jan 29 '24 21:01 victormihalache

The downsides could include:

  • permanent additional complexity
  • the possibility of bugs caused by not using the same argument between nvm invocations
  • the possibility of bugs caused by not using the same argument across machines

and i'm sure all sorts of other unknowns.

The first of these applies to any new feature ofc.

ljharb avatar Jan 29 '24 22:01 ljharb

Still don't get how having an optional way to select a path will break anyone's workflow. If you want a path other than .nvmrc you would be able to use it, if not—as per default in #3235—you can just keep using .nvmrc.

victormihalache avatar May 25 '24 07:05 victormihalache

It won't break anyone's workflow who doesn't use that option, certainly! The issue is that by using the option, you're potentially (likely) breaking other people's workflows.

ljharb avatar May 28 '24 07:05 ljharb