node-version as fallback of node-version-file

[y-nk]

Description

Currently the behavior is that node-version is preferred rather than node-version-file if both are provided.

setup-node/src/main.ts

Lines 85 to 95 in d86ebcd

	if (version && versionFileInput) {
	core.warning(
	'Both node-version and node-version-file inputs are specified, only node-version will be used'
	);
	}
	if (version) {
	return version;
	}
	if (versionFileInput) {

I would like to propose the opposite since node-version is actually hardcoded in the action parameters, but the file is not guaranteed to be there.

setup-node/src/util.ts

Lines 8 to 12 in d86ebcd

	if (!fs.existsSync(versionFilePath)) {
	throw new Error(
	`The specified node version file at: ${versionFilePath} does not exist`
	);
	}

Currently, scenarios resolved as following:

with:
  node-version: 8 # nothing to see here

with:
  node-version-file: fileExists.json # all good

with:
  node-version-file: fileDoesNotExists.json # throws error and crashes the pipeline

with:
  node-version: 8
  node-version-file: fileExists.json # ignored, picked version is 8

with:
  node-version: 8
  node-version-file: fileDoesNotExists.json # ignored, picked version is 8

What I'm requesting for is a change in the 2 lasts scenarios:

with:
  node-version: 8
  node-version-file: fileExists.json # has priority, so it's picked

with:
  node-version: 8
  node-version-file: fileDoesNotExists.json # does not throw and picks version 8 instead

Justification

This is just a nicer API, i think. it does not remove the case of conscious crash of the pipeline if no node-version is given, but it does nicely propose a fail-safe for people interested.

Note

We could also have another parameter such as node-version-fallback-if-file-not-found but it'd just increase the complexity of the api.

Are you willing to submit a PR?

Yes, i'm ok with that.

[aparnajyothi-y]

Hello @y-nk
Thank you for creating this issue, we will investigate it and come back to you as soon as we have some feedback.

[fregante]

To me this feels backwards; generally, config files are the default and "inline settings/flags" override the defaults. If you're specifying a node-version, then you most likely prefer that one over the "project-level" version defined in the package.json

[y-nk]

@fregante that is untrue, especially in monorepos where the node-version file can be there (or not) depending on the package/service running the job, but you could always use a hardcoded base version as a fallback.

[fregante]

I don’t think "monorepos" are the standard. I'm thinking more about CLIs and literally everything else:

1. Use call flag
2. Use local config
3. Use global config
4. Use default

I don’t think it should be anything but that.

[y-nk]

@fregante i understand your standpoint. as a mean not to remain unblocked, would you have any recommendation for something less verbose/more elegant than this:

    - if: ${{ !inputs.workspaceRelativePath }}
      uses: actions/setup-node@v4
      with:
        node-version: 20
        cache: 'pnpm'
        registry-url: https://npm.pkg.github.com/
        scope: '@company'

    - if: ${{ inputs.workspaceRelativePath }}
      uses: actions/setup-node@v4
      with:
        node-version-file: ${{ format('{0}/.node-version', inputs.workspaceRelativePath) }}
        cache: 'pnpm'
        registry-url: https://npm.pkg.github.com/
        scope: '@company'

?

[mk-pmb]

I agree with @fregante in that I expect my explicitly given parameter in the CI workflow file to have more weight than the files in the repo. My CI workflow is there to keep repo file changes in check. Maybe we can have a weak option fallback-node-version; albeit I'd prefer combining all of the possibilities into just node-version as a priority list. You could then specify something like >=16; file:package.json; file:.nvmrc; ; 0; 18; 20 that works like this:

• >=16 sets a mandatory requirement. If our decision would conflict with that, it's a fatal error.
• file:package.json if possible, decide by this file and ignore further entries.
• file:.nvmrc if possible, decide by this file and ignore further entries.
• (empty) ignore. The calling workflow probably had a variable insertion here that happened to not be used in this case.
• 0: Similar to empty but probably as result of a math operation with empty inputs. For convenience we should ignore literal 0; if someone really wants 0.x they can add the .x or prepend a =.
• 18 a literal version. Since this decision mechanism will always succeed, the list will effectively end here.
• 20 probably an accident since this can never be reached. We could consider warning users about this, or silently ignore it because the previous 18 may have been from an optional parameter like the empty slot was.

Update: I don't fully understand the PATH problem in #912, but it seems to in some way be a version number oracle, so we shoudl probably support it here as well. Name could be path or existing (not sure if I understood the intent enough).