Skip to content
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

The behavior of Cache-Control #4985

Open
SukkaW opened this issue Jun 4, 2020 · 3 comments
Open

The behavior of Cache-Control #4985

SukkaW opened this issue Jun 4, 2020 · 3 comments
Labels
documentation Improvements or additions to documentation kv-asset-handler Relating to @cloudflare/kv-asset-handler

Comments

@SukkaW
Copy link
Contributor

SukkaW commented Jun 4, 2020

I have a few question about Cache-Control behavior:

1. Will kv-asset-handler respect header set after getAssetFromKV?

const response = await getAssetFromKV(event,{}); // Passed in an empty object as an option
response.headers.set('cache-control', 'public, max-age=3600');

Will the file be cached at Cloudflare Edge for 2 days (default cache behavior) or an hour?

If the answer is an hour (which means the later set cache-control is used),

const response = await getAssetFromKV(event,{}); // Passed in an empty object as an option
response.headers.set('cache-control', 'public, max-age=3600, s-max-age=4800');

Will the file be cached ar Cloudflare Edge for 80 minutes or an hour?

2. How to use options.cacheControl as a function?

When trying to find the answer of the above question, I went through the code and found this:

https://github.com/cloudflare/kv-asset-handler/blob/eb120a4b789282e4c44ff8b95e2dd2e220b83e6d/src/index.ts#L129-L135

It is never mentioned in the README. Should it be considered not to use in production (Not mentioned in docs thus the behavior might change in the future)?

3. Should s-max-age be implemented for caches.put inside kv-asset-handler?

https://github.com/cloudflare/kv-asset-handler/blob/eb120a4b789282e4c44ff8b95e2dd2e220b83e6d/src/index.ts#L177-L181

It seems that putting cache into Cloudflare Edge is done by kv-asset-handler (Might answer question 1? The headers set later won't be respected?). As far as I know, Cloudflare Edge respect both max-age & s-max-age, and the s-max-age takes higher priority (max-age for browser, s-max-age for CDN).

With s-max-age is used, there might be no need for those Cache-Control manipulation strategy:

https://github.com/cloudflare/kv-asset-handler/blob/eb120a4b789282e4c44ff8b95e2dd2e220b83e6d/src/index.ts#L184-L189

(Is it trying to override edgeTtl with browserTtl, and then even delete Cache-Control if the file shouldn't be cached by browser?)

@surjikal
Copy link

This is still all very confusing for me, and the docs are sparse regarding best practices for caching.

@SukkaW
Copy link
Contributor Author

SukkaW commented May 1, 2021

This is still all very confusing for me, and the docs are sparse regarding best practices for caching.

@surjikal I have already given up this sh***y package. The package shouldn't be used under any circumstances (especially don't use it in your production):

image

If you still want to use the Cloudflare Workers Page, DO NOT use kv-assets-handler, implement your own kv handler instead.

@kristianfreeman
Copy link
Contributor

hello @SukkaW and @surjikal! I'm working on getting this repo updated this week and beginning to resolve some of the long-standing issues here - I appreciate you both taking the time to write in this ticket. the vast majority of people on our team who maintained this project no longer work at CF, so it's been in a bit of limbo since then, sorry about that!

that being said - although this project has some documentation issues and could use some more clarity, especially around things like caching, it is very much safe to use in production. we've been using it heavily in our marketing and documentation sites in the workers/pages ecosystem. I absolutely understand your frustration, but just want to add some perspective for anyone else who might stumble upon this issue 👍

with that out of the way - let's work on how we can make this better! I'm going to remove this from our upcoming milestone release, which will focus on cloudflare/kv-asset-handler#133 and some other related fixes. if there are people who have been involved in this issue and have since dived deeper into caching work, I'd love to work with you on building better documentation around the things you've mentioned in this issue.

we have a #workers-sites channel in our Discord server that would be a great place to sync up on this stuff too: here's an invite link if anyone would like to come in and chat. thanks!

@kristianfreeman kristianfreeman added the documentation Improvements or additions to documentation label May 17, 2021
@GregBrimble GregBrimble transferred this issue from cloudflare/kv-asset-handler Feb 9, 2024
@GregBrimble GregBrimble added the kv-asset-handler Relating to @cloudflare/kv-asset-handler label Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation kv-asset-handler Relating to @cloudflare/kv-asset-handler
Projects
Status: Untriaged
Development

No branches or pull requests

4 participants