Description:
As I understand the documentation about caching both in this repo and in the upstream actions/cache repo, the reason node_modules is not recommended to cache is because dependencies can break across versions of Node.js.
If the Node.js version changing is the only reason not to cache node_modules can this action take into account the node-version-file (or maybe even the node-version)? If it doesn't exist, don't cache the node_modules directory. If it does exist, cache the node_modules directory but use the value as part of the primaryKey:
|
const primaryKey = `node-cache-${platform}-${packageManager}-${fileHash}`; |
Maybe as like:
const primaryKey = `node-cache-${platform}-${nodeVersionFileHash}-${packageManager}-${fileHash}`;If the Node.js version changes, the cache should be invalidated and everything rebuilt. Which seems to address the concern the actions/cache repo points out (as I understand it).
Justification:
The current caching strategy doesn't provide a noticeable improvement for non-trivial projects. It'll shave a couple seconds off of a multiple minute build due to needing to download all of the node_modules directory. Ideally, by caching node_modules when it appears safe to do so, the download of node_modules shouldn't need to happen as frequently and caching can save minutes.
Are you willing to submit a PR?
Yes!
Description:
As I understand the documentation about caching both in this repo and in the upstream
actions/cacherepo, the reasonnode_modulesis not recommended to cache is because dependencies can break across versions of Node.js.If the Node.js version changing is the only reason not to cache
node_modulescan this action take into account thenode-version-file(or maybe even thenode-version)? If it doesn't exist, don't cache thenode_modulesdirectory. If it does exist, cache thenode_modulesdirectory but use the value as part of theprimaryKey:setup-node/src/cache-restore.ts
Line 39 in f099707
Maybe as like:
If the Node.js version changes, the cache should be invalidated and everything rebuilt. Which seems to address the concern the
actions/cacherepo points out (as I understand it).Justification:
The current caching strategy doesn't provide a noticeable improvement for non-trivial projects. It'll shave a couple seconds off of a multiple minute build due to needing to download all of the
node_modulesdirectory. Ideally, by cachingnode_moduleswhen it appears safe to do so, the download ofnode_modulesshouldn't need to happen as frequently and caching can save minutes.Are you willing to submit a PR?
Yes!