[🐞] CacheControl s-maxage and stale-while-revalidate not applied on vercel edge

[maiieul]

Which component is affected?

Qwik City (routing)

Describe the bug

Using cacheControl in a RequestHandler, vercel edge doesn't apply staleWhileRevalidate and sMaxAge properties.

On their docs:

If you set Cache-Control without a CDN-Cache-Control, the Vercel Edge Network strips s-maxage and stale-while-revalidate from the response before sending it to the browser. To determine if the response was served from the cache, check the x-vercel-cache header in the response.

For example, with code from the basic template:

  cacheControl({
    // Always serve a cached response by default, up to a week stale
    staleWhileRevalidate: 60 * 60 * 24 * 7,
    // Max once every 5 seconds, revalidate on the server to get a fresh version of this page
    maxAge: 5,
  });

Vercel edge will only apply max-age: 5 to the header.

Screenshot:

Deployed demo: https://qwik-vercel-edge-cache-trim-issue.vercel.app/demo/todolist/

My current way to solve this issue is to use event.headers.set twice with Cache-Control and CDN-Cache-Control:

    event.headers.set(
      "Cache-Control",
      "public, max-age=60, s-maxage=120, stale-while-revalidate=86400"
    );
    // needed for vercel-edge to not trim s-maxage and stale-while-revalidate headers https://vercel.com/docs/edge-network/caching#cdn-cache-control
    event.headers.set(
      "CDN-Cache-Control",
      "public, max-age=60, s-maxage=120, stale-while-revalidate=86400"
    );

Potential solutions

I'd love to make a PR for this, but I'm not sure what would be the best approach to solve this since we can already work around this issue with headers.set.

Here are my propositions:

1)

Add headers.set documentation to https://qwik.builder.io/docs/caching/

2)

Add

headers.set('CDN-Cache-Control', createCacheControl(cacheControl));

in

   cacheControl: (cacheControl) => {
     check();
     headers.set('Cache-Control', createCacheControl(cacheControl));
   },

which would give

   cacheControl: (cacheControl) => {
     check();
     headers.set('CDN-Cache-Control', createCacheControl(cacheControl));
     headers.set('Cache-Control', createCacheControl(cacheControl));
   },

in packages/qwik-city/middleware/request-handler/request-event.ts

3)

Add a new eventHandler prop:

  CDNcacheControl: (cacheControl) => {
    check();
    headers.set('CDN-Cache-Control', createCacheControl(cacheControl));
  },

in packages/qwik-city/middleware/request-handler/request-event.ts

Either way, I think 1 would be beneficial. 2 would make an easier-to-use API and could end up in a better DX. It may be what the developer will be looking for most of the time and will work with most deployment solutions. 3 would give a bit more control. I think it would be good to do 1 and 2, because I think developers will expect cacheControl to work properly out of the box with vercel edge.
But the problem with 2 and 3 is that they would not account for other Cache-Control variations like Vercel-CDN-Cache-Control and any other variation that exists or will exist (I don't know all of them). What do you think about supporting other variations @mhevery?

Reproduction

https://github.com/maiieul/qwik-vercel-edge-cache-trim-issue

Steps to reproduce

1. pnpm install
2. deploy on vercel edge

System Info

System:
    OS: Linux 6.4 Fedora Linux 38 (Workstation Edition)
    CPU: (16) x64 13th Gen Intel(R) Core(TM) i5-1340P
    Memory: 16.07 GB / 31.05 GB
    Container: Yes
    Shell: 3.6.1 - /usr/bin/fish
  Binaries:
    Node: 18.16.1 - /usr/local/bin/node
    npm: 9.7.2 - /usr/local/bin/npm
    pnpm: 8.7.4 - ~/.local/share/pnpm/pnpm
  Browsers:
    Chrome: 116.0.5845.179
  npmPackages:
    @builder.io/qwik: ^1.2.12 => 1.2.12 
    @builder.io/qwik-city: ^1.2.12 => 1.2.12 
    undici: 5.22.1 => 5.22.1 
    vite: 4.4.7 => 4.4.7

Additional Information

No response

[mhevery]

Thanks for the detailed analysis. Yes, PR would be most welcome.

I am not an expert in the nuances of different edge providers, so I am unsure about the best way to do this.

We could add this to vercel adopter only?

[maiieul]

I have already started working on this. I have come up with a fourth alternative :

    cacheControl: (
      options: CacheControl['options'],
      target: CacheControl['target'] = 'Cache-Control'
    ) => {
      check();
      headers.set(target, createCacheControl(options));
    },

This will allow using cacheControl just like before for backwards compatibility

cacheControl({
  maxAge: 300,
  sMaxAge: 600,
});

Or with the optional parameter

cacheControl({
  maxAge: 300,
  sMaxAge: 600,
}, "CDN-Cache-Control");

This way, it works for any target imaginable.

[mhevery]

Sweet! Looking forward to a PR.