Skip to content

Conversation

priyagupta108
Copy link
Contributor

@priyagupta108 priyagupta108 commented Sep 22, 2025

Description:
This PR updates the caching logic to refine the automatic caching feature added in response to Feature Request #686 and implemented in #1348. In addition, it updates workflow configurations and documentation for improved clarity and compatibility.

Key Changes:

  • Automatic Caching Logic:
    • npm: Caching is now automatically enabled by default if package.json contains either a devEngines.packageManager field or a top-level packageManager field set to npm, and no explicit cache input is provided.
    • pnpm & yarn: Caching is disabled by default to avoid compatibility issues. Users who wish to enable caching for pnpm or yarn can do so manually via the cache input.
  • Workflow Updates:
    • Updated Node.js version specifications in workflow YAML files.
    • Replaced macos-13 with macos-latest-large in workflows, following actions/runner-images#13046.
  • Documentation:
    • Revised documentation to reflect the updated default caching behavior for npm.
    • Updated example workflows to use the latest versions and recommended configurations.

Related issues:

Check list:

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

@Copilot Copilot AI review requested due to automatic review settings September 22, 2025 10:03
@priyagupta108 priyagupta108 requested a review from a team as a code owner September 22, 2025 10:03
@priyagupta108 priyagupta108 self-assigned this Sep 22, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the caching behavior to enable automatic caching only for npm by default, while removing auto-caching support for pnpm and yarn to avoid compatibility issues. The changes also update documentation examples to use Node.js version 24.

  • Restricts automatic caching to npm only when detected from package.json's packageManager field
  • Updates documentation examples from older Node.js versions (14, 16, 18, 20) to version 24
  • Improves error messaging to be more specific about npm auto-caching

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/main.ts Restricts auto-caching logic to npm only and updates related comments
docs/advanced-usage.md Updates Node.js version examples from legacy versions to v24
action.yml Updates package-manager-cache input description to clarify npm-only behavior
tests/main.test.ts Updates test to use npm instead of yarn for package manager detection
README.md Updates documentation to reflect npm-only auto-caching and Node.js v24 examples

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@priyagupta108 priyagupta108 changed the title Update caching defaults for npm and documentation update Limit automatic caching to npm and update documentation Sep 22, 2025
@kachkaev
Copy link

kachkaev commented Sep 22, 2025

I believe this change needs to be released as v6. I have already removed explicit caching option for a few projects and they work well because actions/setup-node@v5 follows pnpm/action-setup@v4. If cache: pnpm becomes a requirement again in v5, existing workflows will end up burning more CI minutes.

@voxpelli
Copy link

I would really love if something like my PR can get in as well, been sitting there for 3 years with very little attention: #702

@scottohara
Copy link

In the context of npm only, I'm interested in whether packageManager is the right field to use for this.

My concern stems from the fact that there are multiple ways to specify a package manager and version, and they all slightly differ in their intended purpose:

engines.<npm|pnpm|yarn>

According to npm's documentation, the engines field in package.json is described as follows:

You can specify the version of node that your stuff works on.
You can also use the "engines" field to specify which versions of npm are capable of properly installing your program.

The key phrasing here seems to be "....capable of properly installing your program".

devEngines.packageManager

According to npm's documentation, the devEngines field in package.json is described as follows:

The devEngines field aids engineers working on a codebase to all be using the same tooling.
Note: engines and devEngines differ in object shape. They also function very differently. engines is designed to alert the user when a dependency uses a differening npm or node version that the project it's being used in, whereas devEngines is used to alert people interacting with the source code of a project.

The key phrasing here seems to be "...people interacting with the source code of a project".

packageManager

This field is not defined or documented by npm, but is rather a nodejs concept introduced in v16.16.0 alongside Corepack.

As with Corepack, it is still marked as experimental, and from Node v25, Corepack is expected to stop being distributed by Node.

Summary

Given all of the above, my take on this is:

devEngines.packageManager would be inappropriate to use in this context, as setup-node does not exactly fit the definition of "...people interacting with the source code of a project"

packageManager is an experimental field used to support Corepack, and indeed the docs specifically state that:

While npm is a valid option in the packageManager property, the lack of shim will cause the global npm to be used.

(so it could be argued that packageManager: npm is not really intended for use? It is certainly not listed in supported package managers.)

engines.npm is npm's official way of specifying the version of npm supported, and setup-node does seem to fit more with the definition of "...capable of properly installing your program".

Anecdotally, Heroku notes in their documentation:

  • For specifying npm, use engines.npm (they don't support packageManager: npm@<version>)
  • For specifying pnpm, use either packageManager: pnpm@<version> or engines.pnpm
  • For specifying yarn, use either packageManager: yarn@<version> or engines.yarn

My view is that setup-node should look in the engines field first to determine which package manager is used (based on the presence of engines.<npm|pnpm|yarn>), and then fallback to packageManager.

I don't think packageManager: npm is the right long term choice here, given the fact that Corepack (and by extension, the packageManager field itself) is likely to be split out from Node in the future.

That is my 2c.

@MikeMcC399
Copy link

@priyagupta108
Copy link
Contributor Author

Hello,

As part of this PR, we introduce detection support for devEngines.packageManager, enabling automatic caching for npm dependencies. This improvement will be included in the upcoming v5 patch release. We believe this enhancement will help streamline workflows and improve consistency across your projects.

@scottohara 👋 , thank you for your thoughtful and detailed feedback. Your explanation of the differences between the engines, devEngines, and packageManager fields has been extremely helpful.

Based on community feedback, we started support for devEngines.packageManager and packageManager to address the needs of teams relying on these fields.

We recognize that engines.npm is the officially recommended way to specify the required npm version, and your insights underscore the importance of aligning with community best practices. In response to ongoing feedback and broader requirements, we will plan to add detection for engines.npm in a future update.

Your feedback and suggestions are always appreciated. Thank you!

@mrgrain
Copy link

mrgrain commented Sep 26, 2025

  • npm: Caching is now automatically enabled by default if package.json contains a packageManager field set to npm and no explicit cache input is provided.
  • pnpm & yarn: Caching is disabled by default to avoid compatibility issues. Users who wish to enable caching for pnpm or yarn can do so manually via the cache input.

This inconsistency between package managers just seems not worth it to me. 🤷🏻 Together with the concerns raised in #1358 I still suggest to just go back on automatic cache enablement.

@kleinfreund
Copy link

@scottohara While I agree that pkg.packageManager is the wrong thing to use to infer whether npm is used (the field doesn't mean anything to npm), I disagree with your analysis on why pkg.devEngines shouldn't be used as part of the inference logic that's being discussed here.

The key phrasing here seems to be "...people interacting with the source code of a project".

❌ devEngines.packageManager would be inappropriate to use in this context, as setup-node does not exactly fit the definition of "...people interacting with the source code of a project"

That is actually what GitHub Actions workflows almost categorically do when we're talking about projects based on Node.js. A workflow typically checks out a GitHub repository (that's what source code means here) to perform some tasks (e.g. run the project's linters or test suite(s)). To optimize for runner storage and workflow execution time, caching node_modules/ and package manager things is useful. And that generally happens in the context of a project's source code. I'm not saying a workflow never npm installs a package that is being published through that repository (in which case you do look at pkg.engines and not pkg.devEngines, but that shouldn't be the common case. In fact, I'd argue that is so rare as to not consider it here.

So, pkg.devEngines is in my opinion definitely the more appropriate field to look at. More reliable still is whether package-lock.json exists next to package.json. pkg.engines is fine as a fallback, but for operations dealing with source code, pkg.devEngines should be considered the correct modern replacement.

setup-node does seem to fit more with the definition of "...capable of properly installing your program".

In that documentation, installing your program refers to running npm install your-program or running npm install on a project that already contains your-program as a dependency. In other words, pkg.engines is about what Node.js version is supported when executing/running the program and as such is checked during installation of it as a dependency in another project.

@alexaka1
Copy link

@scottohara

That is not how I interpret the engines field. The details are lost in the fine print.

Unless the user has set the engine-strict config flag, this field is advisory only and will only produce warnings when your package is installed as a dependency.

This field is primarily used for consuming packages. Ie. I distribute a package called foobar on NPM, and anyone who wants to install it must conform to MY engines property. I can give suggestions to downstream consumers on what version of node my package runs on. I have never seen this used for standardizing tool version in a dev team. This has always been used as a lower and upper bound for packages. But this hasn't been too relevant since TypeScript went viral.

packageManager is a Corepack feature. To me, this is the standardized tool versioning property. It can define the package manager version and automatically install if it is not installed.

devEngines was supposed to be the replacement for Corepack after Node decided to kill it.

So if you're gonna look at any property, it should be devEngines as that is what is supposed to control tools versioning (such as in CI). engines should NOT be used at all.

@Julusian
Copy link

Julusian commented Sep 26, 2025

^ Adding some examples to this, in most of my yarn based projects, the engines is either not set or only includes node version ranges.

The only one of the 3 properties that is set is packageManager, but as that is a corepack property, once/if yarn makes their own entrypoint for yarn, I would expect to see that property disappear. Maybe they will use devEngines instead, or maybe they will add something to their .yarnrc.yml file.
So to be futureproof, any deetection should not be trying to rule out other package managers and assuming npm, but instead confirm that the project is npm

I also have an unusual case to consider, in one project I have:

"engines": {
    "npm": "please-use-yarn",
    "yarn": "^4.5",
    "node": ">=22.19.0 <23"
  },

because inexperienced users kept not reading the docs and trying to use npm then ask for help with the confusing output. It wouldnt be terrible if this was misclassified, but it would be nice to not have to fight the action on this

@kachkaev
Copy link

kachkaev commented Sep 26, 2025

This improvement will be included in the upcoming v5 patch release.
from #1374 (comment)

Please release it as a new major (v6). See #1374 (comment) for details.

@scottohara
Copy link

This inconsistency between package managers just seems not worth it to me. 🤷🏻 Together with the concerns raised in #1358 I still suggest to just go back on automatic cache enablement.

Given the general disagreement above on the purpose and intent of the various different fields and ways a package manager might be specified (by the author) or inferred (by this action), I tend to agree that this seems to be causing more issues than it solves.

Unless automatic cache enablement could be made to work flawlessly with any package manager, and without requiring specific fields in the manifest, then a return to manual cache enablement seems like the best path forward here (that is, return to v4 behaviour).

@MikeMcC399
Copy link

@priyagupta108 / @scottohara

Unless automatic cache enablement could be made to work flawlessly with any package manager, and without requiring specific fields in the manifest, then a return to manual cache enablement seems like the best path forward here (that is, return to v4 behaviour).

Perhaps this needs to be a separate issue / proposal? It seems that this PR is going ahead with enabling caching for npm by default, despite the arguments against this.

@futursolo
Copy link

In my opinion, since both yarn and pnpm essentially recommend corepack as the recommended way to install them,
it would make sense to simply honour the packageManager field via corepack even after Node 25 removes it by default.

This is regardless of whether caching behaviour is automatic or manual.

The last couple comments in #1357 summarised this issue quite well.

Without built in corepack support (or package manager versioning support), workflows with yarn / pnpm basically becomes:

setup-node -> corepack / manually install package manager -> setup-node (cache)

This is a highly undesirable approach for anyone using yarn or pnpm.
Hence, setup-node should manage this internally if it tries to interact with yarn or pnpm.

As for how to check what package manager is used, the action can simply let corepack decide what is the package manager with corepack install.

If the caching is manual, then package managers is already specified and this action can simply call corepack yarn/pnpm.

include:
- os: windows-2016
node_version: 12
- os: windows-2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe now windows-2025?
since this is now windows-latest

- More specific versions: `10.15`, `16.15.1` , `18.4.0`
- Major versions: `22`, `24`
- More specific versions: `20.19`, `22.17.1` , `24.8.0`
- NVM LTS syntax: `lts/erbium`, `lts/fermium`, `lts/*`, `lts/-n`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- NVM LTS syntax: `lts/erbium`, `lts/fermium`, `lts/*`, `lts/-n`
- NVM LTS syntax: `lts/iron`, `lts/jod`, `lts/*`, `lts/-n`

Use codenames of currently supported Node.js versions.
See https://nodejs.org/en/about/previous-releases#looking-for-the-latest-release-of-a-version-branch

@priyagupta108 priyagupta108 changed the title Limit automatic caching to npm and update documentation Limit Caching Automation to npm, Update Workflows and Documentation Oct 3, 2025
@priyagupta108 priyagupta108 changed the title Limit Caching Automation to npm, Update Workflows and Documentation Limit automatic caching to npm, update workflows and documentation Oct 3, 2025
@mrgrain
Copy link

mrgrain commented Oct 3, 2025

@priyagupta108 / @scottohara

Unless automatic cache enablement could be made to work flawlessly with any package manager, and without requiring specific fields in the manifest, then a return to manual cache enablement seems like the best path forward here (that is, return to v4 behaviour).

Perhaps this needs to be a separate issue / proposal? It seems that this PR is going ahead with enabling caching for npm by default, despite the arguments against this.

IDK. This has already been raised in a number of issues and now on this PR. Seems to me the maintainers are aware of the suggestion, but have prioritised a different goal (automated caching for npm) as more important than any of the raised concerns. Which is absolutely their prerogative to do.

Examples:
#1357 (comment)
#1358 (comment)

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

Successfully merging this pull request may close these issues.

10 participants