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

@W-13718763: use s-maxage to only cache shared cache #1564

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

alexvuong
Copy link
Collaborator

Description

The max-age directive implies that a page can be cached both by the browser and the shared CDN cache.

When a new bundle is deployed to an environment, the shared CDN cache is cleared, but stale copies of the site might still exist in a shopper's browser.

We could avoid this situation by moving to s-maxage, which only caches in shared caches:

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#s-maxage

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • (change1)

How to Test-Drive This PR

  • (step1)

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@alexvuong alexvuong added the ready for review PR is ready to be reviewed label Nov 24, 2023
@alexvuong alexvuong self-assigned this Nov 24, 2023
@alexvuong alexvuong requested a review from a team as a code owner November 24, 2023 21:19
kevinxh
kevinxh previously approved these changes Nov 24, 2023
@vcua-mobify
Copy link
Contributor

This PR addresses #1319

I deployed these changes onto a test environment: https://scaffold-pwa-test-env-2.mobify-storefront.com/

I can see the s-maxage appear on the home page.
Screenshot 2023-11-24 at 2 03 07 PM

But on both the PDP and PLP, I see both a maxage and s-maxage
Screenshot 2023-11-24 at 2 03 56 PM
Screenshot 2023-11-24 at 2 04 12 PM

s-maxage overrides maxage where it can but I think we're trying to remove maxage entirely from these pages?

Copy link
Collaborator

@adamraya adamraya left a comment

Choose a reason for hiding this comment

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

I did a quick search of our default Cache Control directives and noticed that a few resources are still using "max-age".

Should we consider updating the Cache Control headers directive to use "s-maxage" instead of "max-age"?

Copy link
Collaborator

@adamraya adamraya left a comment

Choose a reason for hiding this comment

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

Also, remember to add a new entry to the Changelog.

@vcua-mobify
Copy link
Contributor

Approving. In my previous comment, I was looking at the .js file. Looking at the document, I only see s-maxage

Screenshot 2023-11-24 at 3 14 26 PM
Screenshot 2023-11-24 at 3 14 45 PM

@vcua-mobify vcua-mobify merged commit 5254c9b into develop Nov 24, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants