-
-
Notifications
You must be signed in to change notification settings - Fork 8k
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
Fall back to .node-version if it exists #1625
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per #794 (comment), I still think this feature should be blocked on nodejs/version-management#13
README.md
Outdated
@@ -343,6 +344,15 @@ Found '/path/to/project/.nvmrc' with version <5.9> | |||
Now using node v5.9.1 (npm v3.7.3) | |||
``` | |||
|
|||
### .node-version | |||
|
|||
For a little compatability with other node version managers, nvm will also sniff for `.node-version` files. They're the same as `.rmvrc`, they just share a common name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compatibility
also there's no need to mention rvm; node and ruby don't have as much overlap as you might think :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant nvm not rvm, sorry! :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in that case, they're not actually the same - nvmrc supports any "versionish" that nvm does; node-version only would support a literal major.minor.patch version.
README.md
Outdated
|
||
For a little compatability with other node version managers, nvm will also sniff for `.node-version` files. They're the same as `.rmvrc`, they just share a common name. | ||
|
||
$ echo "5.9" > .node-version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the linked issue, i think .node-version
might need to be a specific version - including the patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean? nodenv doesn't require a patch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless, node-version will be a subset of nvmrc - so it'll be important to be specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear here, nodenv is also exact-match with the .node-version string. The only munging that it does, is to allow the addition of a v
prefix.
Patch-less (or even major-only) version specifiers is only supported via the nodenv-aliases plugin. (And that's because the aliases are created by means of symlinks; ergo, as far as nodenv core is concerned, the exact-match name rules still apply.)
nvm.sh
Outdated
nvm_find_nvmrc() { | ||
local dir | ||
dir="$(nvm_find_up '.nvmrc')" | ||
if [ -e "${dir}/.nvmrc" ]; then | ||
nvm_echo "${dir}/.nvmrc" | ||
else | ||
dir="$(nvm_find_up '.node-version')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The semantic here then would be "if no nvmrc is found anywhere, look for a node-version" - i'd expect nvm_find_up
to need to take multiple filenames.
In other words:
.nvmrc
.node-version
$PWD
in $PWD, I'd expect the node-version file to take precedence.
Whereas in:
.nvmrc
.node-version
.nvmrc
$PWD
I'd expect the inner .nvmrc to take precedence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
nvm.sh
Outdated
return 1 | ||
fi | ||
read -r NVM_RC_VERSION < "${NVMRC_PATH}" || printf '' | ||
if [ ! -n "${NVM_RC_VERSION}" ]; then | ||
nvm_err "Warning: empty .nvmrc file found at \"${NVMRC_PATH}\"" | ||
nvm_err "Warning: empty nvm file found at \"${NVMRC_PATH}\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd either list both filenames here, or, track which filename it was found in, and use the correct filename here (ideal)
README.md
Outdated
|
||
$ echo "5.9" > .node-version | ||
|
||
They'll be loaded after `.nvmrc`, and can contain the same values as `.nvmrc`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"same values" can't be correct; nvmrc supports both custom and built-in aliases, and node-version doesn't.
Also, nvmrc can have "iojs-" prefixes; but node-version can't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please offer replacement text for this. I'm not a nvmrc user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
Unlike
.nvmrc
,.node-version
can not contain a “versionish” (an alias, like “node”, “iojs”, or a custom alias you’ve defined)..node-version
can only have versions in the format ofv1
,v1.2
, orv1.2.3
(the “v” is optional).
nvm.sh
Outdated
if [ ! -f "${path}/${1}" ]; then | ||
file="${path}/${1}" | ||
echo $file | ||
break 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it will echo the file twice - instead, should this return 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It used to echo when it found it and now it echos when it finds it. I can return instead of breaking, but it didn't originally return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, that would imply that it always echoed twice, which seems weird
I've updated this with your great suggestions. We'll see what happens with nodejs/version-management#13 |
Will this ever be merged? I really want this to avoid having duplicated files because some people use nvs and others nvm |
Unfortunately @philsturgeon appears to have deleted the forked repo, so this PR can now never be recovered. |
The commits can still be recovered from this repo's copy of the branch (that's why the diff in this PR is still visible). If you'd like I would be happy to recreate the PR, preserving authorship of the commits, so that it may be wrapped up and merged if/when nodejs/version-management#13 is resolved. |
the commits can, but the PR can’t. |
I don't understand what you mean. The changes themselves can be recovered, just not on this particular PR — I think we agree on that. My question is, would you like a replica PR to be created from the commits of this one, or would you rather not? |
@waldyrious at this point no, let's wait until it's unblocked. The issue I was referring to is that every PR makes an eternal, undeleteable git ref in a repo. |
Version normalization removes the issue of the missing v nicely!
The test shows that precedence is maintained, and I've updated the README too.
Resolves #794