-
Notifications
You must be signed in to change notification settings - Fork 126
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
Perf Reduce s3 calls #295
Perf Reduce s3 calls #295
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
// delete potential page data if we're redirecting | ||
await this.deleteS3Objects(key); | ||
await this.putS3Object(key, "redirect", JSON.stringify(data)); | ||
// // delete potential page data if we're redirecting |
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.
This comment may be confusing, since the command is PUT
and not DELETE
. Perhaps you can provide more context?
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.
My bad, i forgot to remove the comment
@@ -523,7 +508,7 @@ export default class S3Cache { | |||
|
|||
private async deleteS3Objects(key: string) { | |||
try { | |||
const regex = new RegExp(`\.(json|rsc|html|body|meta|fetch|redirect)$`); | |||
const regex = new RegExp(`\.(fetch|cache)$`); |
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.
I think this is handled in Derek's PR but the \.
should have been \\.
to escape the RegExp constructor. Since there's no dynamic values here, it might be better just to use the inline one, eg: /\.(fetch|cache)$/
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.
I deployed w/ this branch and everything looks good.
There are a few conflicts from Derek's PR though
8614c66
to
8e5375c
Compare
* fix: fetchCache for next 14+ * fix: regression from #295, tags were not generated * test: add test for revalidateTag * pin next version * add changeset
The goal of this PR is to reduce the number of S3 call that we need to make for the S3 incremental cache
Before this PR there was 1
ListObjectsV2Command
and 2 to 3GetObjectCommand
for everyget
of the incremental cache, this has been reduced to a singleGetObjectCommand
For the
set
part of the incremental cache, we go from 2-3PutObjectCommand
to a single one.This also reduce the number of objects in the S3 cache
TODO: