Respect package.json's engines.node field when used as a node-version-file

[mannie-exe]

Description:
Allow passing package.json files as node-version-file. I'm not 100% sure about how this affects nested packages or workspaces, and any input on the matter would be lovely! I did my best at writing a new test, but my knowledge about Jest is very limited — and any review for that would be appreciated as well.

I'm checking if the file contents of node-version-file are parseable as JSON, and if the engines.node key exists — otherwise the file is treated as it was in the past. If a name-based solution would be better (check if the node-version-file is named package.json explicitly in the installer file), then let me know and I'll make those changes as well.

Thanks for your work!

Related issue:
Adds support for #467

Check list:

• Mark if documentation changes are required.
• Mark if tests were added or updated to cover the changes.

[e-korolevskii]

Hi @schmannie,

Unfortunately, one of the tests fails: "setup-node › node-version-file flag › reads package.json as node-version-file if provided" gives an error when run on Windows. Hopefully you can figure this out.

Have a nice day!

[mannie-exe]

Hey @e-korolevskii !

I can't see the logs from the test any more (at least not on mobile, while I'm away from my computer).

My guess is that the windows test failed because of one of two reasons: path-related issues or a line-ending related issue. I'm guessing it's the line-endings, because:

I simply set the expected arguments in the test to be the raw contents of the JSON file. If a Windows machine with Git set to checkout Windows-styled line-endings is used to run the tests, the newline character sequence will be different when compared to the raw string I've set in the test.

I considered just reading the package.json directly in the test to compare against, but I wasn't sure if that was a useful way to test.

---

How would you suggest I rewrite this test? Since the version file parsing function accepts the file contents directly, I figured just reading the contents of package.json in the test would be a passing solution, but maybe a dumb one. Would it be better to split the functionality or better to just read the package.json in the test as well?

[e-korolevskii]

Hello, @schmannie

As you suggested, tests fails because of line-ending symbols. The one of the easiest way to solve that - replace unwanted "\r" characters. I think, after "readFileSync" (line 91) you could just call the replace method to avoid this problem.
Could you please implement that?

[mannie-exe]

@e-korolevskii sorry it took so long! I moved recently, and somehow it ended my SSD's life lol. A week later and I've finally been able to make the very small change you wanted me to make.

The tests are passing on my end. Let me know if there's anything else you'd like me to do.

[dmitry-shibanov]

Hello @schmannie. Could you please sync with the main branch ?

[mannie-exe]

@dmitry-shibanov all done!

[dmitry-shibanov]

@schmannie Could you please run the npm run format command ?

[mannie-exe]

Hey! I already noticed I forgot to run that. I was just about to submit the last commit — and my whole house's power went out because somebody hit a nearby pole lol. It's gonna take another couple hours... See ya' then hahaha.

---

EDIT: I've made (hopefully) the final push.
Thanks!

---

EDIT: How come the e2e tests failed this time?

---

EDIT: I see just one particular e2e test (node 12 on macOS) failed due to a timeout. Can I get a rerun, or is it safe to merge?

[mannie-exe]

@e-korolevskii @dmitry-shibanov

The tests reran and completed successfully! Pinging one last time, for a final review and merge request.

[sztadii]

Thanks, @schmannie I was looking today for this functionality :) @dmitry-shibanov @marko-zivic-93 can we merge it?

[sztadii]

Ohh, right. Merge conflicts :(

[dmitry-shibanov]

Hello @schmannie. Sorry for the late response. Could you please resolve merge conflicts ?

[mannie-exe]

Hello @schmannie. Sorry for the late response. Could you please resolve merge conflicts ?

Hey there! I'm really sorry, but I haven't had the moment to fix said conflicts. Porting my solution over directly didn't work as expected.

I haven't had the moment to look through the couple months of commits, and the changes they introduced. I noticed there's a different format to how node version files are read? (node[v|version]x.y.z-platform or something) Is this a standard I don't know about? Previously, I was simply returning the engines.node field/the semver in the package.json (if present) — is something else required?

I'll push my changes so far, and we can look at the failing tests together.

EDIT: Just to be clear, I'm not sure of what the issues are or how to read these tests. These changes should not be merged until the issues are fixed, but I would appreciate any help/suggestions.

[brcrista]

These changes should not be merged until the issues are fixed, but I would appreciate any help/suggestions.

@schmannie looks like tests are passing and there are no more merge conflicts. Anything left before we can merge?

[sztadii]

Actually, I thought this behavior should be a default one, cause engines is a part of npm.
So instead of

uses: actions/setup-node@v3
with:
  node-version-file: 'package.json'

we should have

uses: actions/setup-node@v3

[mannie-exe]

These changes should not be merged until the issues are fixed, but I would appreciate any help/suggestions.

@schmannie looks like tests are passing and there are no more merge conflicts. Anything left before we can merge?

I guess not! I was getting a ton of failing tests in my local repo, but I guess I'm glad they're not present. I'm okay with the merge as long as everything looks fine on your end.

Actually, I thought this behavior should be a default one, cause engines is a part of npm.
So instead of

uses: actions/setup-node@v3
with:
  node-version-file: 'package.json'

we should have

uses: actions/setup-node@v3

I think that decision is slightly above my pay grade, but I do sort of agree with this. Either way, changing default behavior should probably be a separate issue.

Thanks everyone!

[brcrista]

Agreed, let's merge this on its own before considering changes to the default behavior.

[brcrista]

Thanks @schmannie !

[sztadii]

@brcrista It was merged 8 days ago and still it is not released :(

[brcrista]

You can reference the SHA of the merge commit directly if you want to use it before the next release comes out.

[sztadii]

@brcrista I am not sure but something went wrong.
When I was using the .nvmrc file then action without any issue downloaded the proper node version.
Please look here to see it working

When I am trying to use the latest version of our then it can not fetch a proper node version.

• actions/setup-node@SHA
• actions/setup-node@main

Any idea what can be wrong?

[vsafonkin]

@sztadii, looks like this regular expression doesn't work correctly with package.json file and the first brace in the file is matched by the expression.

Probably we should skip this regexp check if we use package.json.

[mannie-exe]

@sztadii, looks like this regular expression doesn't work correctly with package.json file and the first brace in the file is matched by the expression.

Probably we should skip this regexp check if we use package.json.

Thank you very much for #553!

[twiddler]

Up until now I used

strategy:
    matrix:
        node: ['14', '16', '18']
steps:
    - uses: actions/checkout@v2
    - uses: actions/setup-node@v2
      with:
          node-version: ${{ matrix.node }}

in my action to test on multiple node versions. Can I use node-version-file: 'package.json' in a way that I define

"engines": {
    "node": "^14 || ^16  || ^18"
},

in package.json and still get a test matrix in my action?

Thanks in advance! 🙏

[XDoubleU]

Up until now I used

strategy:
    matrix:
        node: ['14', '16', '18']
steps:
    - uses: actions/checkout@v2
    - uses: actions/setup-node@v2
      with:
          node-version: ${{ matrix.node }}

in my action to test on multiple node versions. Can I use node-version-file: 'package.json' in a way that I define

"engines": {
    "node": "^14 || ^16  || ^18"
},

in package.json and still get a test matrix in my action?

Thanks in advance! 🙏

@twiddler I want to achieve this too, did you manage to do this? I don't think this belongs inside the scope of setup-node (since strategy matrix is not defined inside the steps of an action) but maybe you have some workaround.

[twiddler]

@XDoubleU No, I haven't. I think a workaround would overcomplicate things, at least if it involved generating github actions with scripts that jq stuff from package.json. And I can't think of anything smarter right now. 🤔