Skip to content
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

[errors/no-cache] Prevent GitHub Actions cache from going stale #27362

Merged
merged 2 commits into from
Jul 22, 2021

Conversation

roim
Copy link
Contributor

@roim roim commented Jul 21, 2021

Closes: #27129

This configuration is similar to what we've been using at Statsig, and it's the best general purpose config I could think of.

At Statsig we also invalidate our caches weekly, since sometimes we spend weeks without updating dependencies for some of our NextJS apps, and the artifact bloat means GitHub starts invalidating other caches in our repo. I could add that step to this diff as well, but I think that's a bit too specific (likely only a problem in monorepos with several caches). In any case, the previous example also has this problem.

Documentation / Examples

  • Make sure the linting passes

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Invalidating when any source file changes seems a little too eager to invalidate the cache as the .next/cache folder is meant to speed up build times significantly.

When leveraging webpack 5 the .next/cache folder shouldn't grow too fast and should automatically invalidate. Are you seeing certain cached assets not be invalidated correctly?

@roim
Copy link
Contributor Author

roim commented Jul 22, 2021

Invalidating when any source file changes seems a little too eager to invalidate the cache as the .next/cache folder is meant to speed up build times significantly.

When leveraging webpack 5 the .next/cache folder shouldn't grow too fast and should automatically invalidate. Are you seeing certain cached assets not be invalidated correctly?

@ijjk

This isn't invalidating any cache. The GitHub cache action works a bit differently than other CI frameworks--it doesn't support cache invalidation to begin with (yes, that is a bit confusing and is why the mistake here is fairly common).

This diff simply ensures that new caches are generated--the old ones are still available and in fact are used to generate the new ones (the restore-keys piece). With the current configuration, a cache is generated when package-lock.json changes, and then it never is updated until that file changes again.

There are other alternatives for when to update the cache. I wouldn't mind doing it every couple hours, or every n commits, etc. Do you think some other strategy is better? The tradeoff is that until the cache is updated, new builds will keep using the stale one (+ build time; might not reuse optimized artifacts), but they also won't have to post the updated cache which takes a few seconds for every couple MB (- build time).

@roim roim requested a review from ijjk July 22, 2021 00:10
Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

Ah I see, thanks for clearing that up! I missed that the cache wasn't updated here unless the key varied, we definitely want to be able to update the cache and fallback to previous ones when not available so this change does look good.

@kodiakhq kodiakhq bot merged commit a3b2205 into vercel:canary Jul 22, 2021
@belgattitude
Copy link
Contributor

belgattitude commented Jul 28, 2021

@roim

  # Generate a new cache whenever packages or source files change.
  key: ${{ runner.os }}-nextjs-${{ hashFiles('**/package-lock.json') }}-${{ hashFiles('**.[jt]sx?') }}
  # If source files changed but packages didn't, rebuild from a prior cache.
  restore-keys: |
    ${{ runner.os }}-nextjs-${{ hashFiles('**/package-lock.json') }}-

Just wondering, in the key part would it make sense to add ie: json, svg... files as well when we import them in the code ? (ie: key: ${{ runner.os }}-nextjs-${{ hashFiles('**/package-lock.json') }}-${{ hashFiles('**.[jt]sx?')-${{ hashFiles('**/src/**.json') }})

Also I would add a comment for people using yarn.lock

@roim
Copy link
Contributor Author

roim commented Jul 28, 2021

@belgattitude I'm not sure. I looked briefly into what was on .next/cache in my projects and I think it's just transpiled, minified js keyed by md5. If so, only files that would change your produced js would be relevant--hopefully someone more experienced can confirm.

Note that you can add extra files in the same hashFiles call, like ${{ hashFiles('**/package-lock.json', 'extra-glob-*) }}. You can merge the last 2 calls in your sample (but not the first one since you need it separately for restore-keys).

Regardless, the updated config here will be strictly better if you have json/svg files imported.

I thought about a comment for yarn/pnpn, but none of the other CI configs have it either. I don't think it's a big deal--most people using either are already used to swapping package-lock.json in their configs.

@belgattitude
Copy link
Contributor

Note that you can add extra files in the same hashFiles call, like ${{ hashFiles('**/package-lock.json', 'extra-glob-*) }}. You can merge the last 2 calls in your sample (but not the first one since you need it separately for restore-keys).

Thanks for this tip 😄

I thought about a comment for yarn/pnpn, but none of the other CI configs have it either. I don't think it's a big deal--most people using either are already used to swapping package-lock.json in their configs.

Agree.

flybayer pushed a commit to blitz-js/next.js that referenced this pull request Aug 19, 2021
…el#27362)

Closes: vercel#27129

This configuration is similar to what we've been using at Statsig, and it's the best general purpose config I could think of.

At Statsig we also invalidate our caches weekly, since sometimes we spend weeks without updating dependencies for some of our NextJS apps, and the artifact bloat means GitHub starts invalidating other caches in our repo. I could add that step to this diff as well, but I think that's a bit too specific (likely only a problem in monorepos with several caches). In any case, the previous example also has this problem.

## Documentation / Examples

- [x] Make sure the linting passes
@vercel vercel locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggested GitHub Actions caching gets stale
3 participants