Github Actions templates cache doesn't hit due to hashing bug

[felixmosh]

Current Behavior

Due to the fact that this line has a glob, it will create a wrong key!
Explanation, when the workflow starts it doesn't have any node_modules, therefore hashFiles('**/yarn.lock') will use only the package's yarn.lock (as expected), but at the end of the flow, node_modules are installed, hashFiles('**/yarn.lock') will contain all yarn.lock files from node_modules as well, therfore hashFiles('**/yarn.lock') will save the cache on a different key.

Expected behavior

It should relay only on the package's yarn.lock file.

Suggested solution(s)

Remove the glob from this line https://github.com/formium/tsdx/blob/master/templates/basic/.github/workflows/main.yml#L20.

Additional context

Based on the research that @AllanChain did, AllanChain/blog#98

Your environment

Software	Version(s)
TSDX
TypeScript
Browser
npm/Yarn
Node
Operating System

[agilgur5]

Has this issue been reported and confirmed upstream? Because **/yarn.lock and **/lockFiles more generically is what GitHub officially recommends in the cache docs, both at the @actions/cache repo docs and the GitHub Actions docs site

[felixmosh]

I've tested it (using debug) and saw the list...

[agilgur5]

I mean that doesn't answer the question... I would think there is a reason GitHub used **/lockFile over the easier and more intuitive lockFile, meaning there's a potential issue with that as well.

Before saying "x is wrong, downstream should change" it would be good to hear what the upstream authors think. This is far from the only repo that uses that pattern given it is the official pattern

[felixmosh]

Ok, I will try to approach them as well.

[AllanChain]

I haven't done much test and just quoted @felixmosh in the blog post, which means I assume actions/cache will not hit due to the glob.

As for the reason for using glob, in my opinion, is in case of mono repo which contains multiple lock files. Besides, **/lockfile is supposed to work, because libraries should not include lock file. Unfortunately, some libraries still include lock file, leading to this bug.

[agilgur5]

mono repo which contains multiple lock files

I can't say I know much about JS monorepos, but it seems like they actually don't have multiple lock files per #778 / lerna/lerna#2105 / yarnpkg/yarn#5428

They also use that pattern for all other languages too, even though monorepos are by far most common in JS because of the available tooling.

because libraries should not include lock file.

package-lock.json also cannot be included by NPM in publishes.
But looks like the same isn't true for Yarn which doesn't specifically exclude yarn.lock and even says it's harmless to publish 😕

[felixmosh]

You should commit your lock files, the question is if it is published to the npm repo.

From my experience, I've debugged the github action, and saw the differences.

https://npm.community/t/yarn-lock-not-published-by-npm-publish-anymore/7855/4

[agilgur5]

You should commit your lock files, the question is if it is published to the npm repo.

Yes, that's what I was talking about. The NPM link I added says they cannot be published, so the inclusion of lockfiles in publishes is specific to Yarn

[AllanChain]

@agilgur5 You are right. But monorepo is the only case I can think of. Maybe lock file in example/test project makes sense?

package-lock.json also cannot be included by NPM in publishes.

I don't know why, but I am getting package-lock.json in babel-runtime...

[agilgur5]

I've migrated the templates to use bahmutov/npm-install in #882 which doesn't have this issue since it hashes just the one lockfile. I've also contributed several changes there myself and might be added as a maintainer there.

I'm still interested in seeing what the GitHub folks have to say upstream in actions/cache#400 though, but they unfortunately haven't responded in over a month 😕

[agilgur5]

I don't know why, but I am getting package-lock.json in babel-runtime...

Huh, checked babel-runtime's published files and it is indeed there. Weird... not sure how it got there... It does seem to use npmignore instead of package.json#files though 🤷
It's not in @babel/runtime's published files though.
TSDX v0.14.0 (to be released some time today/tomorrow) has upgraded all deps which had the old Babel 6 babel-runtime fwiw.

Maybe lock file in example/test project makes sense?

That's a possibility, though neither example nor test should be published, but I can certainly see authors accidentally including them. I'd think NPM's ignore applies to all package-lock.jsons in the repo though 😕

[agilgur5]

@allcontributors please add @felixmosh for bug

[allcontributors]

@agilgur5

I've put up a pull request to add @felixmosh! 🎉