feat(go): add response caching

[thebongy]

No description provided.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@cloudflare/flagship@27

commit: a5d2f69

[thebongy]

/bonk review this PR

[ask-bonk]

Review Summary

Clean, well-structured addition of opt-in TTL + LRU response caching to the Go provider. The implementation is consistent with the caching patterns already established in the TypeScript and Python SDKs, all tests pass, and the edge cases are handled correctly.

What looks good

• Opt-in design: Cache is nil by default, zero overhead when not enabled. Consistent with the TS/Python SDK pattern.
• Cache key correctness: buildCacheKey uses contextToQueryParams which delegates to url.Values.Encode(), which sorts keys alphabetically — cache keys are deterministic regardless of Go map iteration order.
• Type safety in cache keys: Including expectedType in the key prevents a cached int response from being served for a float evaluation (or vice versa) of the same flag. Matches the TS SDK approach.
• Selective caching: Errors, disabled flags, and type mismatches are correctly excluded from the cache. The cache-store happens only after successful type conversion, which is the right placement.
• Concurrency: sync.Mutex on the cache is appropriate. The LRU implementation using container/list + map is idiomatic Go and avoids external dependencies.
• Lifecycle: Shutdown clears the cache. ShutdownWithContext delegates to Shutdown.
• Test coverage: 9 focused tests covering: disabled-by-default, cache hit with CachedReason, context-keyed isolation, type-keyed isolation, TTL expiry, LRU eviction, disabled-flag exclusion, error exclusion, type-mismatch exclusion, and shutdown clearing.
• Documentation: README updates and option table additions are clear and complete.
• Changeset: Correctly targets @cloudflare/flagship-go with a minor bump.

Minor observations (non-blocking)

1. buildCacheKey error silently skips caching (provider.go:144): When buildCacheKey returns an error (invalid context), cacheKey stays empty and the evaluation proceeds without caching. This is fine because the same invalid context will also cause EvaluateFlat to fail, so there is nothing to cache anyway. Just noting this is intentional.

2. Cache stores the raw EvaluationResponse including json.Number (provider.go:194): The cached EvaluationResponse.Value is the raw any from JSON parsing (e.g., json.Number). On cache hit, it is passed through the same convert function again. This works correctly, but it does mean the convert function runs on every cache hit. The overhead is negligible, and it keeps the code simple. The alternative (caching the converted value) would require a more complex generic cache, so this is the right trade-off.

3. No max TTL cap: Unlike Retries (capped at 10) and RetryDelay (capped at 30s), CacheTTL has no upper bound. A user could set CacheTTL: 24 * time.Hour and get very stale values. This is arguably fine since it is explicitly opt-in and documented as "values may be up to this stale," but worth considering whether a cap (e.g., 5 minutes) would be a useful guardrail in a future iteration.

LGTM — nice work.

[ask-bonk]

Review posted with approval on PR #27. Here's a summary of the findings:

Overall: Clean implementation that adds opt-in TTL + LRU response caching to the Go provider, consistent with the existing caching patterns in the TypeScript and Python SDKs. All 9 new tests pass alongside the existing test suite.

Key strengths:

• Zero overhead when caching is disabled (cache is nil by default)
• Deterministic cache keys via url.Values.Encode() (alphabetically sorted)
• Type-safe cache keys prevent cross-type cache hits (e.g., int vs float)
• Errors, disabled flags, and type mismatches are correctly excluded from caching
• Idiomatic Go LRU using container/list + map, no external dependencies
• Thorough test coverage across all edge cases

Minor observations (non-blocking):

1. buildCacheKey errors silently skip caching — correct behavior since invalid context will also fail the evaluation
2. Raw json.Number values are cached and re-converted on each hit — negligible overhead, keeps code simple
3. No upper cap on CacheTTL unlike other options — fine for now since it's explicitly opt-in

github run