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

Skip cache logic when mode is no-store #964

Merged
merged 2 commits into from
Jun 1, 2023
Merged

Skip cache logic when mode is no-store #964

merged 2 commits into from
Jun 1, 2023

Conversation

frandiox
Copy link
Contributor

We were still running all the cache logic when mode is no-store. This PR changes it to skip the whole cache logic when this mode is specified.

This change means we are not reading from cache anymore in this mode. Technically, the "no-store" only signals "don't write", not necessarily "don't read and return an existing cached response". But I think this might be OK?
Or should we keep reading (checking for existing responses) and only skip writing? That would only be useful when the mode changes across requests depending on some condition... not sure if this would ever be desired.

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

@frandiox frandiox requested review from wizardlyhel and a team May 30, 2023 00:36
@github-actions

This comment has been minimized.

Copy link
Contributor

@blittle blittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be bad to also skip reading the cache too?

@juanpprieto
Copy link
Contributor

That would only be useful when the mode changes across requests depending on some condition... not sure if this would ever be desired.

I can imagine this being a desired use case when working with 3rd party CMS data

@frandiox
Copy link
Contributor Author

Would it be bad to also skip reading the cache too?

This PR skips reading for CacheNone. Trying to find a use case where this is not desirable.

I can imagine this being a desired use case when working with 3rd party CMS data

Hm.. any more specific example that use the same cache-key? I'm having a hard time thinking of a situation where a CacheNone-query should reuse a previously cached response 🤔

@blittle
Copy link
Contributor

blittle commented May 31, 2023

I think this is a good chance. Also cache checks are a sub-request from CF's perspective, so essentially every storefront sub-request results in at least two sub-requests. With this change, no cache check means no extra sub-request.

@frandiox
Copy link
Contributor Author

frandiox commented Jun 1, 2023

Merging this for now 👍

@frandiox frandiox merged commit 8ccf6db into 2023-04 Jun 1, 2023
@frandiox frandiox deleted the fd-no-cache branch June 1, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants