feat(cache): add cache key helper auto-including provider name and options#239
feat(cache): add cache key helper auto-including provider name and options#239sushichan044 wants to merge 7 commits intounjs:mainfrom
Conversation
florian-lefebvre
left a comment
There was a problem hiding this comment.
Looks good overall! Just a comment on the API of cacheKey
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
|
@florian-lefebvre |
Why is that? |
|
I was thinking about the possibility that we wouldn’t be able to add any new options later. That said, it’s unlikely that we’ll need to add options to this function, so I think it’s fine to ignore that concern. |
|
Ah gotcha. Also unifont is in pre v1 so breaking changes can occur at any time. @danielroe I'd like your review on this when you have time |
| const unifontContext: ProviderContext = { | ||
| storage, | ||
| cacheKey: createCacheKeyFactory(provider._name, provider._providerOptions), | ||
| } |
There was a problem hiding this comment.
could we do this automatically rather than exposing the cacheKey to the provider? ie. automatically setting the cacheKey as the first part of any key the provider accesses?
There was a problem hiding this comment.
@danielroe I think we can do that by wrapping storage interface. I'll try it.
There was a problem hiding this comment.
There are two reasons why the cacheKey helper is exposed to the provider.
One, as you mentioned, is to automatically account for the provider name and options — this can be done automatically by changing the internal implementation.
The other is to allow users to safely generate keys simply by passing the data they want to use as the key source, without manually performing hashing or string concatenation.
To achieve this, I believe we would need to modify the interface of ProviderContext["storage"].
If we decide to proceed, it would likely require a separate PR.
e.g.
const fonts = await ctx.storage.getItem(
["data.json", fontFamily, defaults], // automatically hashed and concatinated
() => getFontDetails(fontFamily, defaults),
)There was a problem hiding this comment.
Anyway, I’d like to split out only reason 1 into a separate PR.
There was a problem hiding this comment.
@danielroe @florian-lefebvre
I’ve created a draft implementation in #281 that automatically includes the provider name and options in the cache key.
Could you take a look when you have time?
|
Closing this PR since the design was changed in #281. |
resolves #184
This PR introduces a unified cache key factory that automatically includes provider name and options in generated cache keys, ensuring cache invalidation when provider configuration changes.
All built-in providers have been updated to use the new cache key factory.
Problem
Previously, cache keys were not provider-aware, leading to stale cache reuse when:
experimental.glyphsin Google provider)Additional Information
I should be add documentation later to support implementing a custom provider,
including safe ways to cache key generation.