feat(node): Add experimental FlagDefinitionCacheProvider
#2594
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Most packages are already resolving Jest versions from the catalog anyway. This allows better synchronization (node was on Jest 29 but using types for Jest 28).
This interface is used to provide custom caching logic for feature flag definitions used by local eval.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
Contributor
Contributor
|
Size Change: +6.8 kB (+0.13%) Total Size: 5.06 MB
ℹ️ View Unchanged
|
This exposes experimental types subject to change in minor versions, e.g., FlagDefinitionCacheProvider
dustinbyrne
force-pushed
the
feat/node-flag-definition-cache
branch
from
ca64f97 to
c4ee36c
Compare
dustinbyrne
commented
Nov 18, 2025
| * @returns true if this instance should fetch, false to skip and read cache | ||
| * @throws if coordination backend is unavailable (error will be logged, fetch continues) | ||
| */ | ||
| shouldFetchFlagDefinitions(): Promise<boolean> | boolean |
Contributor
Author
There was a problem hiding this comment.
This initial interface doesn't provide any additional context to the implementation. My justification is that:
- Static information like team id can be provided via the constructor
- We already have configuration for how often we're polling to update cache. If the implementation wants to be smarter about this, they're better off persisting metadata in the cache (such as last fetch time). In a distributed service we can't guarantee things like the last time fetched.
I think there's potentially a case to be made to expose an interface to hit the flag definitions API in the future. For example, it could make HEAD requests and track an ETag or Last-Modified header. Or, maybe that's the context we provide to this method. In any case, we're not quite ready for this and I see this as a future improvement.
dustinbyrne
commented
Nov 18, 2025
Comment on lines
+639
to
+640
| // Important: if `shouldFetchFlagDefinitions` throws, we | ||
| // default to fetching. |
Contributor
Author
There was a problem hiding this comment.
Does this seem reasonable? In the event this method throws (e.g., can't access cache), this would mean all instances resort back to polling directly from the API every interval.
Contributor
There was a problem hiding this comment.
I think so.
If someone wants different behavior, there's a workaround. They could ensure this never throws (ex: with a wrapper cache provider) and instead make sure it returns false instead.
dustinbyrne
commented
Nov 18, 2025
Comment on lines
+1
to
+11
| /** | ||
| * Experimental APIs | ||
| * | ||
| * This module exports experimental features that may change or be removed in minor versions. | ||
| * Use these APIs with caution and be prepared for breaking changes. | ||
| * | ||
| * @packageDocumentation | ||
| * @experimental | ||
| */ | ||
|
|
||
| export type { FlagDefinitionCacheProvider, FlagDefinitionCacheData } from './extensions/feature-flags/cache' |
Contributor
Author
There was a problem hiding this comment.
I marked the interface and config option as @experimental in TSDoc, explaining that it may see breaking changes over minor version bumps. To be really explicit about this, I've also exposed the type under a separate import.
import type { FlagDefinitionCacheProvider } from 'posthog-node/experimental'I'd like to get some real implementations working before we solidify this API.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
16 files reviewed, 1 comment
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
haacked
approved these changes
Nov 18, 2025
Contributor
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Really nice and well thought out.
| * | ||
| * async getFlagDefinitions(): Promise<FlagDefinitionCacheData | undefined> { | ||
| * const cached = await this.redis.get(`posthog:flags:${this.teamKey}`) | ||
| * return cached ? JSON.parse(cached) : undefined |
Contributor
There was a problem hiding this comment.
Folks will copy this. Should we have a try/catch in this example?
Contributor
Author
There was a problem hiding this comment.
I think this is fine, if this throws it'll consider it a cache miss. If the data is malformed it's not really recoverable from here anyhow.
Comment on lines
+639
to
+640
| // Important: if `shouldFetchFlagDefinitions` throws, we | ||
| // default to fetching. |
Contributor
There was a problem hiding this comment.
I think so.
If someone wants different behavior, there's a workaround. They could ensure this never throws (ex: with a wrapper cache provider) and instead make sure it returns false instead.
Comment on lines
+1
to
+11
| /** | ||
| * Experimental APIs | ||
| * | ||
| * This module exports experimental features that may change or be removed in minor versions. | ||
| * Use these APIs with caution and be prepared for breaking changes. | ||
| * | ||
| * @packageDocumentation | ||
| * @experimental | ||
| */ | ||
|
|
||
| export type { FlagDefinitionCacheProvider, FlagDefinitionCacheData } from './extensions/feature-flags/cache' |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
This change introduces a new
FlagDefinitionCacheProviderinterface. This interface allows custom caching strategies to be implemented for feature flag definitions used by local evaluation.Related to PostHog/posthog#37584
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file