Skip to content
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

fix: remove cache-control headers handling in redirect logic #6991

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nelsonprsousa
Copy link
Contributor

@nelsonprsousa nelsonprsousa commented Oct 19, 2024

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

@nelsonprsousa nelsonprsousa requested a review from a team as a code owner October 19, 2024 23:28
Copy link

changeset-bot bot commented Oct 19, 2024

⚠️ No Changeset found

Latest commit: 5de0cdf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link

pkg-pr-new bot commented Oct 19, 2024

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: 5de0cdf

Copy link
Contributor

github-actions bot commented Oct 19, 2024

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

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

@nelsonprsousa
Copy link
Contributor Author

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

@maiieul maiieul added COMP: DX Developer Experience related issue STATUS-2: requires discussion Requires further discussion before moving forward labels Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: DX Developer Experience related issue STATUS-2: requires discussion Requires further discussion before moving forward
Projects
Status: Waiting For Review
Development

Successfully merging this pull request may close these issues.

2 participants