Skip to content

Conversation

@mikehardy
Copy link
Collaborator

Description

How to review this PR - Each commit in this series is a fully separate idea to improve things, and each should be reviewed separately by reading the commit message which explains that specific change then checking the diff for that commit

The basic idea is this though:

  • caches are not visible from branch to main, or between OSs by rule (so you want to cache main but read-only on branch)
  • cache failures shouldn't fail a workflow (it happens, it's a useless flake to fail on)
  • firestore emulators are truly cross-platform so may be shared

Related issues

A companion to the similar PR developed in FlutterFire repo:

The react-native-firebase repo already had many of the important best practices implemented from my previous work here so doesn't have quite as many changes as the FlutterFire PR

Release Summary

test-only

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
    • Other (macOS, web)
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

Best way to check it is to verify cache hit/miss info from the workflow runs themselves, in combination with a look at the LRU status of the caches for the whole repo (do we have old caches indicating our caches are long-lived and thus more useful?)

can look here https://github.com/invertase/react-native-firebase/actions/caches?query=sort%3Aaccessed-asc

Currently we do have some 3-day-old caches which indicates reasonable utility but there are lots of branch-specific caches and 3 days is not that old, so the changes here should help


Think react-native-firebase is great? Please consider supporting the project with any of the below:

use the separate actions/cache "restore" and "save" actions so we may
only save optional based on whether we are on main or not

actions/cache isolates branch caches from main to avoid branch cache
poison attacks on main runs

but branches may see main cache as those caches are considered trustworthy

branch caches do consume available LRU storage though, meaning branch caches
could evict main caches and in high traffic / large cache repositories like
this one that means caches LRU out frequently and cache hit rates are terrible
unless you cache on main only
very occasionally, there are errors in the `hashFiles` function used
to calculate cache keys, and this causes a build error that is a flake,
not a valid CI failure signal

so, continue the workflow even if the caching setup has errors, as
that is a recoverable problem and the workflow may still have a valid
success outcome
the downloaded/cached files are pure javascript and java, they may be
shared across operating systems safely
@vercel
Copy link

vercel bot commented Jan 1, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
react-native-firebase Ready Ready Preview, Comment Jan 1, 2026 3:20pm

@mikehardy mikehardy merged commit 8dbc5ba into main Jan 2, 2026
21 checks passed
@mikehardy mikehardy deleted the @mikehardy/ci-caching-best-practices branch January 2, 2026 13:32
@mikehardy
Copy link
Collaborator Author

only way to verify workflows (esp when they have "only work on main" conditionals) is to merge and see, so I'm going to do that now

If I see anything unexpected I'll post a follow-up plus it will inform changes to the sister PR over in FlutterFire repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants