Revert "fix: solve issue with Cache-Control header deletion (#6991)"

[maiieul]

This reverts commit dea36be. From this PR: https://github.com/QwikDev/qwik/pull/6991/files.

What is it?

• Bug

Description

fixes #7742.

Reminder of the issue – I have a repro with 3 cache layers:

• / -> maxAge: 300

• /one/ -> maxAge: 200

• /one/two/ -> maxAge: 100

• current issue === screenshot 1 === version 1.10 and above -> the /one/ route with a throw redirect(...) is cached when it hits the redirect with it's closest parent Cache-Control (in this case 300 defined in /).

• previous behavior === screenshot 2 === version 1.9.1 and below (about 1 year ago) -> the /one/ gets a Cache-Control value of no store because redirects are not cached by default.

On all versions, the redirect target (/one/two/) get its proper Cache-Control inherited from the closest parent.

So current behavior (screenshot 1) is not good because now a if(token) throw redirect(...) in a middleware/routeLoader$ will always redirect when cached and token is true. This can lead to broken e2e tests that test redirect logic, and can also break end user experiences (user B might get cached redirect from user A if it's a server side cache).

The current workaround is to add a

if(token) {
cacheControl("no-store")
throw redirect(...)

when you don't want to cache redirects. This is not good either because the default behavior should be "no-store" whereas currently it is set to the closest parent Cache-Control value and this is far from being intuitive.

The problem with

      // Fallback to 'no-store' when end user is not managing Cache-Control header
      if (statusCode > 301 && !headers.get('Cache-Control')) {

(introduced in the problematic PR), is that Cache-Control will inherit the value of the closest parent, so headers.get('Cache-Control') will always hold a Chache-Control value from the closest parent unless told otherwise (e.g. in screenshot 1, the maxAge: 300 applied to /one/). As the comment suggests, the Cache-Control will be set to no-store if there is no Cache-Control at all in all the parent layouts (which is rare!). And we can't go the other way around (apply no-store by default and a Cache-Control only if defined on the route, because (1) the route might use its own Cache-Control that shouldn't be applied to the redirect, (2) we can't differentiate it with the layout, and (3) I'm not even sure qwik-city can be refactored to do this because routeLoaders need to be hoisted – and that would probably be a huge refactor either way.

Besides, caching a redirect doesn't seem to boost performance that much because the html is not rendered when the server hits a redirect. The only use-case we could think of was to give redirects a TTL (time to live) even after being removed from the code, but that's also a bad idea. Better change the code exactly when you want to stop the redirect ^^.

Last point: I doubt even 0.01% of qwik apps leverage redirect caching. I tagged @nelsonProusa who made the problematic PR but he has not replied to my issue. And reverting won't break E2Es or end-user experiences since it will just prevent cached redirects.

So I propose we revert for now because it was a regression in terms of E2E tests and end-user experiences. We can then work on an RFC if there is more demand for this feature. This will give us more time to get this right, also considering the eTag logic for V2 routeLoaders.

Ideas for a proposal:

if(token) {
 throw redirect(302, "blablabla", {maxAge: 123}) // `no-store` default now gets overriden by `maxAge: 123`.
}

Checklist

• My code follows the developer guidelines of this project
• I performed a self-review of my own code
• I added a changeset with pnpm change
• I made corresponding changes to the Qwik docs
• I added new tests to cover the fix / functionality

[changeset-bot]

🦋 Changeset detected

Latest commit: a6968af

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages

Name	Type
@builder.io/qwik-city	Patch
eslint-plugin-qwik	Patch
@builder.io/qwik	Patch
create-qwik	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[shairez]

thanks @maiieul , can we add a test for that please? (just to make sure we are not dealing with regressions in the future)

[maiieul]

👌 I added 4 tests. 2 for the core logic and 2 bonus ones :).

[github-actions]

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name	Status	Preview	Last Commit
qwik-docs	✅ Ready (View Log)	Visit Preview	a6968af

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@builder.io/qwik@7811

npm i https://pkg.pr.new/@builder.io/qwik-city@7811

npm i https://pkg.pr.new/eslint-plugin-qwik@7811

npm i https://pkg.pr.new/create-qwik@7811

commit: a6968af