sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.45k stars 1.89k forks source link

Replace `maxage` in `load` with more detailed `cache` option #4549

Closed Rich-Harris closed 2 years ago

Rich-Harris commented 2 years ago

Describe the problem

Pages can return a maxage property from load which a) sets a Cache-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 repeated load.

This mostly works, but has limitations:

Describe the proposed solution

Replace maxage with a cache object:

<script context="module">
  /** @type {import('@sveltejs/kit').Load} */
  export async function load() {
    return {
      props: {...},
      cache: {
        maxage: 3600,
        private: false // if unset, uses existing heuristic
      }
    };
  }
</script>

In future, we could add revalidate and smaxage if those prove necessary.

Alternatives considered

No response

Importance

nice to have

Additional Information

No response

elliott-with-the-longest-name-on-github commented 2 years ago

Hey @Rich-Harris -- would you like me to tackle this one? Happy to grab it if no one else is currently working on it.

Rich-Harris commented 2 years ago

That would be great @tcc-sejohnson, thank you!

elliott-with-the-longest-name-on-github commented 2 years ago

@Rich-Harris

These changes will cause merge conflicts with the existing pull request I have open (#4515) -- the typedef for LoadInput has completely moved and changed to include the ErrorLoadInput properties, and LoadOutput has also moved (though it is unchanged).

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 master and we can just deal with the merge conflicts when we get to them.

elliott-with-the-longest-name-on-github commented 2 years ago

Disregard previous -- I see the PR has been merged. I'll work on this over the weekend and report back with any questions.

elliott-with-the-longest-name-on-github commented 2 years ago

@Rich-Harris

Completion candidate: #4690

I exhaustively updated the tests to cover every edge case I could think of.

elliott-with-the-longest-name-on-github commented 2 years ago

@Rich-Harris

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 :)

andreivreja commented 2 years ago

some people want to be able to express s-maxage rather than maxage, though I'll confess I don't fully understand why

@Rich-Harris I might be missing something obvious but, isn’t s-maxage the only way to cache on the proxy (CDN, Vercel for example)? I do this often on pages that don’t require real time updates since even a 5m edge cache can save a lot of computational power. It’s both a cost effective and performant (UX wise) approach.

I always felt that being able to just pass a custom cache-control header would make everything much easier, since you can avoid dealing with hooks (where, right now, I have to also implement logic for checking if I output a cache header from an endpoint to prevent hooks from overriding it).

elliott-with-the-longest-name-on-github commented 2 years ago

@Rich-Harris

some people want to be able to express s-maxage rather than maxage, though I'll confess I don't fully understand why

I was just reading through the Vercel documentation and I found out why:

Recommended Cache-Control We recommend that you set your cache to have max-age=0, s-maxage=86400, with changing 86400 to the amount of seconds you want your response to be cached for. This recommendation will tell browsers not to cache and let our edge cache the responses and invalidate when your deployments update, so you never have to worry about stale content.

andreivreja commented 2 years ago

@Rich-Harris

some people want to be able to express s-maxage rather than maxage, though I'll confess I don't fully understand why

I was just reading through the Vercel documentation and I found out why:

Recommended Cache-Control We recommend that you set your cache to have max-age=0, s-maxage=86400, with changing 86400 to the amount of seconds you want your response to be cached for. This recommendation will tell browsers not to cache and let our edge cache the responses and invalidate when your deployments update, so you never have to worry about stale content.

Similar scenario with Cloudflare specific cache headers. I ended up handling cache headers in hooks to get the behavior I need.

kristjanmar commented 2 years ago

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?

andreivreja commented 2 years ago

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' };
craigcosmo commented 1 year ago

didn't we have setHeaders already ? why do we need this cache object ?

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 {}
    }
}