Fix browser cache headers from always being set #37
Conversation
| browserTTL: 0, | ||
| edgeTTL: 100 * 60 * 60 * 24, // 100 days | ||
| browserTTL: null, | ||
| edgeTTL: 2 * 60 * 60 * 24, // 2 days |
There was a problem hiding this comment.
lets keep this at 100?
There was a problem hiding this comment.
it's weird IMO because it would never stay on cache for that long and puts us and developers at risks for situations like this.
There was a problem hiding this comment.
cache-control: max-age=undefined
Hey so this doesn't quite work, I'm fiddling around with it
I think if you change the if (options.browserTTL === null) to if if (!options.browserTTL) it seems to work as expected :) should add some tests for this. I am a little confused as to why exactly options.browserTTL was undefined in the first place... since you explicitly set it to null
Fixed |
src/index.js
Outdated
| @@ -121,6 +121,13 @@ const getAssetFromKV = async (event, options) => { | |||
| if (response) { | |||
| let headers = new Headers(response.headers) | |||
| headers.set('CF-Cache-Status', 'HIT') | |||
| if (options.cacheControl.browserTTL === 0 || options.cacheControl.browserTTL) { | |||
There was a problem hiding this comment.
I think we should separate this boolean check to its own function that returns a boolean, and it should also return false if options.cacheControl.browserTTL is negative
There was a problem hiding this comment.
so like:
const toSetBrowserTTL = cacheControl => cacheControl.browserTTL === 0 || cacheControl.browserTTL
? is purpose for readability ?
There was a problem hiding this comment.
readability and also so we don't repeat ourselves, this code is used twice - generally that should be a separate function imo.
const isValidBrowserTTL = browserTTL => {
let ttl = parseInt(browserTTL, 10)
if (typeof ttl !== "number" || ttl < 0) {
return false
}
return true
}
exvuma
force-pushed
the
victoria/cache
branch
from
dabd971
to
c2a6436
Compare
For cache HITs we always returned the edge cache TTLs to the client, which in turn means the browser would be caching as long as the edge does (100 days)
Also, I set the edge cache to a safer, more realistic value (no edge would likely not get close to caching an asset for 100 days)
fixes #38