nvm-sh / nvm

Node Version Manager - POSIX-compliant bash script to manage multiple active node.js versions
MIT License
79.33k stars 7.94k forks source link

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

Open victormihalache opened 10 months ago

victormihalache commented 10 months ago

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 commented 10 months ago

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
}
ljharb commented 10 months ago

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.

victormihalache commented 10 months ago

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.

ljharb commented 10 months ago

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.

victormihalache commented 10 months ago

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)

ljharb commented 10 months ago

"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.

victormihalache commented 10 months ago

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.

ljharb commented 10 months ago

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.

victormihalache commented 10 months ago

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.

fulldecent commented 10 months ago

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.

victormihalache commented 10 months ago

@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

fulldecent commented 10 months ago

Everyone, except NVM already supports the other file name

victormihalache commented 10 months ago

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)

fulldecent commented 10 months ago

Please find extensive documentation and feature matrix here

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

victormihalache commented 10 months ago

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.

ljharb commented 10 months ago

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.

MGLPollockjr commented 9 months ago

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

victormihalache commented 8 months ago

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

ljharb commented 8 months ago

The downsides could include:

and i'm sure all sorts of other unknowns.

The first of these applies to any new feature ofc.

victormihalache commented 4 months ago

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.

ljharb commented 4 months ago

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.