Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
victormihalache opened this issue Nov 21, 2023 · 19 comments

Comments

@victormihalache
Copy link

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

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

ljharb commented Nov 21, 2023

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

victormihalache commented Nov 21, 2023

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

ljharb commented Nov 21, 2023

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

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

ljharb commented Nov 23, 2023

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

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

ljharb commented Nov 24, 2023

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

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

This comment was marked as off-topic.

@victormihalache

This comment was marked as off-topic.

@fulldecent

This comment was marked as off-topic.

@victormihalache

This comment was marked as off-topic.

@fulldecent

This comment was marked as off-topic.

@victormihalache

This comment was marked as off-topic.

@ljharb
Copy link
Member

ljharb commented Dec 1, 2023

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

This comment was marked as spam.

@victormihalache
Copy link
Author

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

@ljharb
Copy link
Member

ljharb commented Jan 29, 2024

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.

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

No branches or pull requests

4 participants