chore: add a requirement to have an optional init option in providers…#74
Conversation
… for prefixing the cache key
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the ADR for local storage in static-context providers to include an optional 'persisted-cache key prefix' configuration. This change aims to prevent storage key collisions when multiple provider instances share the same storage partition, such as a single web origin. The feedback focuses on improving the clarity and logical consistency of the document, specifically by refining the description of how the prefix is used during lookup and ensuring it is correctly categorized as a configuration option rather than a static-context input. Additionally, it was noted that the implementation guidelines should be updated to reflect this new requirement.
| During initialization, a provider should follow a cache-first approach: | ||
|
|
||
| 1. Attempt to load a matching persisted bulk evaluation from local storage (matching `cacheKeyHash`). | ||
| 1. Attempt to load a matching persisted bulk evaluation from local storage (matching `cacheKeyHash`, and the same persisted-cache key prefix the instance was configured with, if any). |
There was a problem hiding this comment.
The phrasing "matching ... (matching ...)" is redundant. Additionally, since the prefix is used to construct the storage key (as defined in the Decision section), it is more accurate to describe it as being used for the lookup rather than being "matched" against the loaded content.
Suggested improvement:
- Attempt to load a persisted bulk evaluation from local storage using the configured persisted-cache key prefix (if any) and the
cacheKeyHash.
|
|
||
| Providers should only reuse a persisted evaluation when it matches the current static-context inputs. | ||
| This includes a matching `cacheKeyHash` equal to `hash(targetingKey)`. | ||
| This includes a matching `cacheKeyHash` equal to `hash(targetingKey)` and a persisted-cache key prefix provided in the initialization options. |
There was a problem hiding this comment.
The persisted-cache key prefix is a configuration option rather than a "static-context input". Grouping it under the requirement that the evaluation "matches the current static-context inputs" (line 141) is logically inconsistent.
Suggested improvement:
This includes a matching cacheKeyHash equal to hash(targetingKey). The lookup must also use the persisted-cache key prefix provided in the initialization options.
There was a problem hiding this comment.
I agree I think this might be in the wrong spot.
|
|
||
| Providers should allow applications to disable the default persistence behavior, for example with a `disableLocalCache` option, or replace the storage backend when platform requirements or policy constraints require it. | ||
|
|
||
| When applications configure **more than one** static-context provider against the same underlying storage (same browser origin, shared app container, and so on), providers should expose an optional **persisted-cache key prefix** (name may vary by SDK, for example `persistedCacheKeyPrefix` or `localCacheKeyPrefix`). Applications set a distinct prefix per provider instance so persisted entries are namespaced and instances do not load or overwrite each other’s bulk evaluations. |
There was a problem hiding this comment.
Since this ADR now requires providers to expose a persisted-cache key prefix option, the guideline/static-context-provider.md file should be updated to include this in its "Configuration" section to maintain consistency between the decision and the implementation guidelines.
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
Approved with a nit, up to you if you want to change it.
You will need to do a git commit --amend --signoff && git push -f to fix the DCO issue.
2 participants
This PR