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

Increase default cache SWR #1336

Merged
merged 9 commits into from Oct 25, 2023
Merged

Increase default cache SWR #1336

merged 9 commits into from Oct 25, 2023

Conversation

benjaminsehl
Copy link
Member

@benjaminsehl benjaminsehl commented Sep 13, 2023

WHY are these changes introduced?

After some inspiration from https://www.youtube.com/watch?v=bfLFHp7Sbkg&t=1035s, and talking to a number of developers who weren't customizing their caching strategy but certainly were not getting more than 1 request per 10 seconds per page, I'd like to propose we extend SWR time by default for all requests to be 1 day.

We would still keep max-age to be 1 second, so the server can go and start to revalidate things right away, but the only downside to this approach from what I see is that flash sellers may have small gaps where inventory availability status is inaccurate — but I believe they should opt to use CacheShort, where I've now changed our default to use a new strategy: CacheDefault.


🤖 Generated by Copilot at 516d829

This pull request changes the default caching strategy for the Hydrogen framework to CacheDefault, which improves the performance and flexibility of caching public resources. It also adds a changeset file to document the patch update for the @shopify/hydrogen package.

WHAT is this pull request doing?

🤖 Generated by Copilot at 516d829

  • Change the default caching strategy for the Hydrogen framework to extend the stale-while-revalidate (SWR) time to 1-day for public resources (link)
  • Define and export a new CacheDefault function that returns the cache options for the default strategy and accepts an optional override object (link, link)
  • Replace the CacheShort function with the CacheDefault function in the getCacheControlSetting, getCacheOption, and fetchStorefrontApi functions that determine the cache options for the Hydrogen API, the sub-requests, and the GraphQL requests respectively (link, link, link)
  • Import the CacheDefault function from the ./strategies module in the ./api, ./sub-request, and ./storefront modules that use it (link, link, link)

HOW to test your changes?

  1. Check response time on cold request
  2. Wait a couple seconds to check response time and make sure it's faster (cache should hit)
  3. Wait >10 seconds and check again, and time should still be fast

Post-merge steps

Merge and public documentation update to indicate change to default caching: https://shopify.dev/docs/custom-storefronts/hydrogen/data-fetching/cache#cache-data-from-third-party-apis, PR here: #1336

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

@benjaminsehl benjaminsehl requested a review from a team September 13, 2023 15:27
@benjaminsehl
Copy link
Member Author

One question I had was whether or not we should make CacheDefault a publicly exposed function or not. You shouldn't ever need to use it — so perhaps not?

@duncan-fairley
Copy link

Why not introduce sMaxAge in caching strategies as well?

@benjaminsehl
Copy link
Member Author

Why not introduce sMaxAge in caching strategies as well?

Hmm … not a bad idea, I'll think about updating this so we keep s-maxage at 1, and then bump out max-age to be a little longer.

That said, with this PR I really wanted to change as little as possible so there shouldn't be any disruption to current behaviour for most folks. If we increase max-age then I could imagine that some folks would complain they're not seeing updates as quickly as they used to.

For now I think we'll keep this as is. Certainly, I'd advocate in calls to third parties to have a higher max-age than s-maxage so you hedge against any quotas you might have — but in Shopify's case we won't throttle you so, in my opinion, there's no real disadvantage for the end-user or for merchants.

@@ -0,0 +1,5 @@
---
'@shopify/hydrogen': patch
Copy link
Contributor

@abecciu abecciu Sep 14, 2023

Choose a reason for hiding this comment

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

Shouldn't this be at least a minor release given that we are changing an important default behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is the weird thing about changesets & calver … right @blittle? "Minor" would mean it gets bumped to 2023.08

This is definitely a DevOps oddity we need to resolve.

Copy link
Contributor

Choose a reason for hiding this comment

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

Using calver, following storefront api release, is a decision we agree on - So everything is a patch. If it is a breaking change, we should release as in opted-in feature or unstable feature until the next calendar release.

Copy link
Contributor

@wizardlyhel wizardlyhel left a comment

Choose a reason for hiding this comment

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

This change will make Hydrogen fast but stale. So that means that devs will need to evaluate what's the risk tolerance for a page being stale (for that 1 second) and adjust accordingly.

@wizardlyhel wizardlyhel changed the base branch from main to 2023-10 October 20, 2023 19:05
@wizardlyhel wizardlyhel mentioned this pull request Oct 20, 2023
12 tasks
Base automatically changed from 2023-10 to main October 25, 2023 21:34
@wizardlyhel wizardlyhel merged commit 867e0b0 into main Oct 25, 2023
9 checks passed
@wizardlyhel wizardlyhel deleted the increase-default-cache-swr branch October 25, 2023 21:58
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

4 participants