-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix computed revision cache in v1 #3142
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
Conversation
|
Summary of the deployments: Version 1
Version 2
Test content |
a30e71c
to
faa4e1b
Compare
faa4e1b
to
eebb5a3
Compare
eebb5a3
to
83ef048
Compare
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.
Looks good, but we need to find a way to preserve all existing cache (99.999% of the site have no openapi).
46cda13
to
61ccfb6
Compare
61ccfb6
to
e9dc6ad
Compare
e9dc6ad
to
f140093
Compare
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
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.
Overall looks good, but we'll have to monitor very closely after deployment, so I'm not approving yet as I'm not on the laptop.
Also it looks like the code is incomplete
f140093
to
e7b7c32
Compare
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.
At this stage, it'll still bust the cache for all spaces
@SamyPesse it's fixed. |
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.
Looks good
4cce632
to
d5e11b8
Compare
d5e11b8
to
617e385
Compare
@@ -440,10 +475,15 @@ export const getRevisionPages = cache({ | |||
|
|||
return cacheResponse(response, { | |||
...(fetchOptions.metadata ? cacheTtl_7days : cacheTtl_1day), | |||
data: response.data.pages, | |||
data: { ...response.data, tags: getResponseCacheTags(response) }, |
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.
Isn't it an array usually ?
As I said @SamyPesse I don't think it's a good idea to do that. |
In v1, we consider revision as immutable but it's not when computed. To mitigate that, we pass some tags when we fetch a computed revision, so it's not immutable.