-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Replace maxage
in load
with more detailed cache
option
#4549
Comments
Hey @Rich-Harris -- would you like me to tackle this one? Happy to grab it if no one else is currently working on it. |
That would be great @tcc-sejohnson, thank you! |
These changes will cause merge conflicts with the existing pull request I have open (#4515) -- the typedef for If you're bullish on that change, I can base the current changes off of the tip of my feature branch to make the eventual merge easier. If you're bearish on it or think it needs a significant rework, I can base the changes off of current |
Disregard previous -- I see the PR has been merged. I'll work on this over the weekend and report back with any questions. |
Completion candidate: #4690 I exhaustively updated the tests to cover every edge case I could think of. |
Glad to see it all worked out (other than the snafu with the doc site -- oops). Just wanted to let you know this issue is still open. I can't close it :) |
@Rich-Harris I might be missing something obvious but, isn’t I always felt that being able to just pass a custom |
I was just reading through the Vercel documentation and I found out why:
|
Similar scenario with Cloudflare specific cache headers. I ended up handling cache headers in hooks to get the behavior I need. |
Do you have a code example for this? |
There's a bunch of ways, depending on the use case. One generic approach I do where I need to cache some routes on Cloudflare based on the route type: // Can use routeId instead of the pathname
if(event.url.pathname.indexOf('/pathtocache') === 0) {
response.headers.set('cloudflare-cdn-cache-control', 'max-age=1600')
} Another use case is where I cache articles but I don't want to cache draft articles: // hooks
const bypassCacheTestArticle = response.headers.get('cache-control')?.includes('somenameheretocheckfor')
if (bypassCacheTestArticle) {
// remove the header we use for the hack
response.headers.set('cache-control', 'no-cache')
response.headers.set('cloudflare-cdn-cache-control', 'no-cache')
} else {
response.headers.set('cloudflare-cdn-cache-control', 'max-age=600')
}
// route
// return the header inside load when the article is in draft mode
response.cache = { maxage: 'somenameheretocheckfor' }; |
didn't we have we already have this export const load: PageServerLoad = async (event: any) => {
const token = event.locals.token
try {
const res = await getDataServer(api.getOrders, token)
event.setHeaders({
'cache-control': 'max-age=5',
})
return res
} catch (error) {
return {}
}
} |
Describe the problem
Pages can return a
maxage
property fromload
which a) sets aCache-Control
header on the server response, and b) causes the returned value to be cached in memory so that navigating back to the page won't result in a repeatedload
.This mostly works, but has limitations:
public
is based on a heuristic (nofetch
withoutcredentials: 'omit'
, and nosession
access) that doesn't cover every edge caserevalidate
s-maxage
rather thanmaxage
, though I'll confess I don't fully understand whyDescribe the proposed solution
Replace
maxage
with acache
object:In future, we could add
revalidate
andsmaxage
if those prove necessary.Alternatives considered
No response
Importance
nice to have
Additional Information
No response
The text was updated successfully, but these errors were encountered: