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 support for comments in version files like .nvmrc #175

Open
atl-mk opened this issue Oct 23, 2020 · 14 comments
Open

Add support for comments in version files like .nvmrc #175

atl-mk opened this issue Oct 23, 2020 · 14 comments

Comments

@atl-mk
Copy link

atl-mk commented Oct 23, 2020

I recently came across a project with a .nvmrc file with a comment in it which is useful to tell maintainers that unfortunately it's not the single source of truth (due to it being duplicated in a pom.xml that uses frontend-maven-plugin). Could support be added to ignore anything after a version number that comes at the start of the file?

cat .nvmrc
12.18.0
# note to update the pom.xml node version too

@jasongin
Copy link
Owner

Does nvm support comments in .nvmrc?

There is a long discussion at nodejs/version-management#13 about agreeing on a standard format for a file that specifies the node version for a project, however there was no consensus reached. Without agreement there, I would hesitate to extend support for more options in that file.

@ljharb
Copy link
Contributor

ljharb commented Oct 23, 2020

Absolutely nvm does not support comments; the only thing it can contain is an nvm-style "version-ish", with perhaps a trailing newline.

@atl-mk
Copy link
Author

atl-mk commented Oct 28, 2020

I've noticed that neither fnm nor nvm have issues processing this particular file. Perhaps that's just due to the implementation details. I understand why you're hesitant to officially support this behaviour.

What about settling for an unofficial support via less strict parsing?

@jasongin
Copy link
Owner

Yes, it would be make sense to ignore any following lines after the first. (No need for a setting.)

@ljharb
Copy link
Contributor

ljharb commented Oct 29, 2020

fwiw I'm far more likely to change nvm to error on extra lines being present, in order to preserve the design space - iow, to make the parsing stricter.

@atl-mk
Copy link
Author

atl-mk commented Oct 29, 2020

@ljharb How about warning for now and then do a breaking change to error (probably at the same time as the rest of the file standard is defined) since the contract isn't currently defined?

I'm imagining this would mean that current users won't have a broken experience as soon as they upgrade nvm, but they can fix the warning by removing whatever other non-standard lines. IMO this gives consumers some advanced notice and thus a better experience, but I'm keen to hear your thoughts.

I do appreciate this adds a little bit of complexity in the mean time.

@jasongin As for how nvs should handle this; aside from eventually reaching consistency in the contract with the version files across all node version manager/switcher tools being important (IMO), I obviously have vested interest here, but unless the nvm's behaviour will actually change soon I don't see any reason to be relatively more strict.

@mtariqsajid
Copy link

please add support for comments i don't get it why it was not done in the early stage. its like very basic requirement of any config file

@ljharb
Copy link
Contributor

ljharb commented Nov 24, 2023

The core config file of the node ecosystem has never had comments, so i don’t think it’s as basic as you’re claiming.

@atl-mk
Copy link
Author

atl-mk commented Nov 28, 2023

The core config file of the node ecosystem has never had comments, so i don’t think it’s as basic as you’re claiming.

It's often called out as a missing feature. The trend within the ecosystem seems to be moving towards more expressive configuration with .json -> .js

@mtariqsajid
Copy link

@ljharb @atl-mk but every config file has comments also known as dotfile

because i need to tell other developer that 18 means is nodejs version 18
its very confusing to just put 18 in .nvmrc file

@ljharb
Copy link
Contributor

ljharb commented Nov 28, 2023

i hear you. I’m just saying that package.json doesn’t have comments and it’s fine.

i don’t personally find it confusing - what else would 18 and “nvm” mean - but that’s subjective.

@mtariqsajid
Copy link

i hear you. I’m just saying that package.json doesn’t have comments and it’s fine.

i don’t personally find it confusing - what else would 18 and “nvm” mean - but that’s subjective.

junior developer

@korpiq-tiketti
Copy link

I would like to add a reason for sticking to a specific node version. In our case, we need the experimental-specifier-resolution option that was removed in node 19, so we're stuck with node 18. I'd love to be able to state this reason where the setting is instead of the README. Can do without; would like to have the comment at the setting it explains.

@DavidBruchmann
Copy link

There are also options to define the version more general than just with a distinct version number.
Explaining the options or the meaning of the used option might be useful.
Some completely unrelated config files come with lengthy explanations as comments, so I don't see a reason to exclude this option from the .nvmrc file format.

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

6 participants