fix: solve issue with Cache-Control header deletion

[nelsonprsousa]

What is it?

• Bug (kinda)

Description

Currently, it is impossible to define a Cache-Control header using the request.cacheControl on any method that takes a request event other than success (HTTP 200). Looks like qwik-city, internally, deletes whatever is defined.

I am not entirely sure about the error() and fail() handlers, which are also forcing the removal of the header. I'd say that engineers could potentially decide to cache 404s, for example, and the framework should not enforce this kind of rules. However, I am not sure about this premise for 4xx or 5xx.

Redirects (HTTP 301, HTTP 302, HTTP 307, HTTP 308, at least) should be totally configurable by software engineers.

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

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

[pkg-pr-new]

Open in Stackblitz

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

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

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

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

commit: d30d2f3

[github-actions]

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

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

[nelsonprsousa]

After a more in-depth analysis, I found that we currently have three methods that are hard deleting user-defined Cache-Control headers:

• redirect
• error
• fail

In my opinion, at least redirect and error should not enforce this rule. For HTTP 301, 302, 307, 308, and possibly other 3xx status codes, it should be the user's responsibility to define the cache control. Additionally, when we do something like throw error(HttpStatusCode.NOT_FOUND, 'Not Found'); (e.g., when the id in a route like url.com/:id does not exist), that response should be cacheable. This means we should remove the hardcoded rule from the error handler as well.

As for the fail handler, I don’t have a strong opinion. It may be used more in the 5xx range, such as for internal server errors, where invalidating the cache could be the right approach.

Looking forward to your feedback @wmertens @thejackshelton @maiieul @mhevery

[gioboa]

Nice, thanks @nelsonprsousa
Can you add a changeset please?(follow this tutorial)

[nelsonprsousa]

Nice, thanks @nelsonprsousa Can you add a changeset please?(follow this tutorial)

Sure, I can.

What do you think about the other handlers that are actually doing the same thing? Particularly error?

fail I am tempted to leave it as is (even though I still think it is quite a bold move from a framework to perform this kind of operations).

[gioboa]

I think is not correct to change that header internally because the end user needs to have the ability to manage it.
So I think we need to be transparent on that and keep the end user one and we can add a fallback to preserve the actual behavior.

[nelsonprsousa]

@gioboa I removed the hard code removal of the Cache-Control header from qwik-city as agreed 👍

Also added a changeset.

LMK if everything's ok 👍

[gioboa]

It looks great to me. Can you add the fallback please?

[gioboa]

Great @nelsonprsousa 👍