Use node version config files if node-version input is omitted

[sndrs]

This PR uses @ehmicky's preferred-node-version to attempt to determine a node version to use if you omit node-version before it falls back to the PATH version.

It checks the following places (edited to reflect @ehmicky's comment below):

1. .n-node-version
2. .naverc
3. .node-version
4. .nodeenvrc
5. .nvmrc (accurately parsing nvm versions)
6. package.json#engines.node

and finally the following environment variables:

7. NODIST_NODE_VERSION
8. NODE_VERSION
9. DEFAULT_NODE_VERSION

We're using this approach at the guardian (in a fork) and it seems to work well.

But we'd rather stay official :)

[ruscon]

can we push forward this feature?

[jasonkarns]

edit: this was all based on an inaccurate assumption which has since been corrected.

I like the idea, but I would be wary of using https://www.npmjs.com/package/preferred-node-version as it stands. Per the documentation for that tool (without any first-hand experience) it seems to prioritize nvm, then package.json and then all other version managers. That's fine for a specific node package like https://www.npmjs.com/package/preferred-node-version to use; but that's inappropriate for github's official node action.

I would suggest the official action should prioritize a version file that isn't version-manager specific (like .node-version which is supported by multiple node version managers), falling back to version-manager-specific files, and then package.json#engines.node's "fuzzy" matching as a last resort. (I would expect package.json to be last because its version specs is intended to contain ranges; and deterministic builds on CI should be preferred)

[ehmicky]

Hi everyone (I maintain that library)!

For reference, this is the current priority order. The current logic prioritizes version manager-specific files because they are more specific. For example, .nvmrc is used by many tools (for example Netlify uses it). If a project has both a .nvmrc and a .naverc file, I would assume .nvmrc to be more likely to be part of the project as "boilerplate" or as part of some sort of global configuration (since parent directories are searched for). This leads to manager-specific files being prioritized.

After those comes package.json engines.node which has lower priority for the reason mentioned by @jasonkarns. Also because its intent is to define the allowed Node.js version for consumers, which might not be the same the Node.js version for developers (which might be more relevant in CI).

Finally comes the environment variables: NODIST_NODE_VERSION, NODE_VERSION and DEFAULT_NODE_VERSION. Those have the lowest priority because they are global and might apply to many projects, i.e. are less specific.

[jasonkarns]

@ehmicky thanks for clarifying and correcting my misinterpretation! I retract my "opposition" to this PR.

My (inaccurate) assumption was based solely on the phrasing of the readme:

This looks for any .nvmrc or package.json [...] This also looks for any [other files] or [variables].

[ehmicky]

One note: the library can throw. For example, if the project has an .nvmrc with an invalid Node.js version (wrong syntax, or Node.js version does not exist). Would it be a good idea not to print the error as a warning message, and not make the build fail when this happens? Or would hard failure be proper in that case?

[nrako]

I believe a hard failure would be expected here. I don't think there's any reason why an invalid nodejs version should fail silently.

[sndrs]

thanks @ehmicky – i'll update the description of the PR to reflect the ordering

[Macavirus]

Is there any blocker to merging this?

[sndrs]

@Macavirus I'm not sure. Currently 2 checks are failing but i've followed the advice and don't understand what the issues are.

I'm also not clear whether GitHub would be interested in merging it either.

@jasonkarns are you able to advise?

In the meantime, we're using https://github.com/guardian/actions-setup-node at the guardian (basically this PR) if that's any help to anyone

[jasonkarns]

@sndrs I'm not a member of the team and do not have any capability to merge or weigh in on the PR more than anyone else here. So far I haven't seen a github team member in this thread.

[sndrs]

@jasonkarns lol sorry! i'm not sure where i got that idea from – sorry to alert you :)

@bryanmacfarlane (because you last merged a PR in the repo – apologies if you too are the wrong person) are you able to advise if this is something that might get merged? if so, any advice on how i can fix the broken checks?

[jasonkarns]

(@sndrs no worries! I was at @github for a time, so quite likely you saw my name associated as an org member)

[AlexWayfer]

@gordey4doronin @maxim-lobanov @dmitry-shibanov no reviews here? 😞

[sndrs]

got confused with working with forks and accidentally closed this. not intentional

[AlexWayfer]

got confused with working with forks and accidentally closed this. not intentional

Oh, nice. There are conflicts, BTW.

[sndrs]

hopefully the conflicts are dealt with and license checks now pass (they pass for our fork at https://github.com/guardian/actions-setup-node and which is still used across the guardian).

be great if someone is able to kick them off? (or just close this PR if it's not going to be merged so i can stop updating it 😄)

[sndrs]

@maxim-lobanov given #338 and that no one from GitHub has ever responded to this PR, should I just close it? getting strong vibes GitHub's not all that interested 😉

[sndrs]

thanks everyone for your input. GitHub are going with their own solution in #338 so I'm closing this.

in case anyone is using the fork I mentioned, we'll archive it when the official action supports version files natively, so it should keep working but won't get upstream updates