-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Limit automatic caching to npm, update workflows and documentation #1374
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
base: main
Are you sure you want to change the base?
Limit automatic caching to npm, update workflows and documentation #1374
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.
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.
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 |
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 |
In the context of 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:
|
|
Hello, As part of this PR, we introduce detection support for @scottohara 👋 , thank you for your thoughtful and detailed feedback. Your explanation of the differences between the Based on community feedback, we started support for We recognize that Your feedback and suggestions are always appreciated. Thank you! |
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. |
@scottohara While I agree that
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 So,
In that documentation, installing your program refers to running |
That is not how I interpret the
This field is primarily used for consuming packages. Ie. I distribute a package called
So if you're gonna look at any property, it should be |
^ 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 I also have an unusual case to consider, in one project I have:
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 |
Please release it as a new major (v6). See #1374 (comment) for details. |
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). |
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. |
In my opinion, since both yarn and pnpm essentially recommend corepack as the recommended way to install them, 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. As for how to check what package manager is used, the action can simply let corepack decide what is the package manager with If the caching is manual, then package managers is already specified and this action can simply call |
include: | ||
- os: windows-2016 | ||
node_version: 12 | ||
- os: windows-2022 |
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.
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` |
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.
- 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
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: |
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:
package.json
contains either adevEngines.packageManager
field or a top-levelpackageManager
field set tonpm
, and no explicitcache
input is provided.pnpm
oryarn
can do so manually via thecache
input.macos-13
withmacos-latest-large
in workflows, following actions/runner-images#13046.Related issues:
package-manager-cache
added #1351pnpm
#1357Check list: