-
Notifications
You must be signed in to change notification settings - Fork 161
Implement persistent data cache option and update cache key logic #920
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 4710f0a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Is the fix related to the other changes? (Maybe clarify in the PR description) There are conflicts. I'll take a look later today |
My bad i rebased on the wrong branch... I'll fix it |
74ae605
to
a8b69ff
Compare
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I added a few minor comments but changes look good.
Probably we should doc that overrides should not use the BUILD_ID to enable the feature.
I question:
For composable cache, we use
globalThis.tagCache.hasBeenRevalidated(
result.value.tags,
result.lastModified,
vs hasBeenRevalidated
from utils
- any particular reason. Would be nice to add comments.
try { | ||
const key = createCacheKey(baseKey, true); | ||
const cachedEntry = await globalThis.incrementalCache.get(key, "fetch"); | ||
|
||
if (cachedEntry?.value === undefined) return null; | ||
|
||
const _tags = [...(tags ?? []), ...(softTags ?? [])]; | ||
const _lastModified = cachedEntry.lastModified ?? Date.now(); | ||
const _hasBeenRevalidated = await hasBeenRevalidated( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a comment explaining why the base key is used (inline + on the method)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-opening.
Could we expand on why this is not using the obj key?
I'll update the PR with the changes later today |
Thought: In the Couldflare adapter the cache key is
That will become harder to do with this PR. There are multiple ways to solve this but while thinking about this I was wondering if the change could not be implemented by only updating the overrides? i.e. we could pass a key that is independent of the BUILD_ID and then up to the override to add the build Id or not. |
That's exactly what was done before (except for the composable cache). One option would be to have a special type as a key like interface CacheKey {
baseKey: string,
buildId?:string
} You can then decide what you do with it. |
+1 It could also help alleviate the confusion between |
|
||
Add an option to keep the data cache persistent between deployments. | ||
|
||
BREAKING CHANGE: Incremental cache keys are now an object of type `CacheKey` instead of a string. The new type includes properties like `baseKey`, `buildId`, and `cacheType`. Build_id is automatically provided according to the cache type and the `dangerous.persistentDataCache` option. Up to the Incremental Cache implementation to use it as they see fit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicitly say that the overrides should be updated?
(Adding this text to the PR description)
await globalThis.incrementalCache.set( | ||
key, | ||
{ | ||
await globalThis.incrementalCache.set(key, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look like this is a formatting only diff?
* @param key The composable cache key | ||
* @returns The composable cache key. | ||
*/ | ||
function getComposableCacheKey(key: string): CacheKey<"composable"> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be merged into createCacheKey?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah probably
try { | ||
const cacheKey = getComposableCacheKey(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to move that l45 and use key
on l42 and 43?
edit:
Oh actually I think that's what we want as they might differ.
Should we add a comment warning that key
should not be used?
edit2:
But I guess that I have the same question as the tag cache: why wouldn't we use the build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it doesn't really matter, it's in memory anyway.
For the tag cache, technically we don't need the build_id as it is time based.
There is one case actually when it is useful to have the build_id, it's for more complex deployment (like blue/green).
I think what make sense is just to pass the CacheKey
to the tag cache as well and let the implementation decides.
For the default one, I think we should just follow the persistentDataCache
option
WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
|
||
const getCacheKey = (key: string) => { | ||
return path.join(basePath, `${key}.cache`); | ||
}; | ||
|
||
const cache: IncrementalCache = { | ||
name: "fs-dev", | ||
get: async (key: string) => { | ||
const fileData = await fs.readFile(getCacheKey(key), "utf-8"); | ||
get: async ({ baseKey }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get: async ({ baseKey }) => { | |
get: async (cacheKey: CacheKey) => { | |
// This cache is always shared across build (the build id is not used) | |
const { baseKey } = cacheKey; |
const { NEXT_BUILD_ID } = process.env; | ||
return `__meta_${NEXT_BUILD_ID}_${key}`; | ||
const buildDynamoKey = (key: CacheKey<CacheEntryType>) => { | ||
return `__meta_${key.buildId ? `${key.buildId}_` : ""}${key.baseKey}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could this be simplified?
not the exact same output as the current code but it should not matter?
return `__meta_${key.buildId ? `${key.buildId}_` : ""}${key.baseKey}`; | |
return `__meta_${key.buildId ?? ""}_${key.baseKey}`; |
cacheType: type, | ||
buildId: undefined, | ||
baseKey: key, | ||
} as CacheKey<CacheType>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is as CacheKey<CacheType>
needed here?
// We always prepend the build ID to the cache key for ISR/SSG cache entry | ||
// For data cache, we only prepend the build ID if the persistentDataCache is not enabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"prepend" do not sound right any more.
would be great to update the comments and move to their corresponding case.
Introduce a new option to maintain the data cache across deployments and adjust cache key generation to reflect this change. Fix issues related to tag caching and ensure proper handling of cache keys in various scenarios.
BREAKING CHANGE: Incremental cache keys are now an object of type
CacheKey
instead of a string. The new type includes properties likebaseKey
,buildId
, andcacheType
. Build_id is automatically provided according to the cache type and thedangerous.persistentDataCache
option. Up to the Incremental Cache implementation to use it as they see fit.