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

AMP HTML documents handled via NetworkFirst strategy #70

Open
gilbertococchi opened this issue Jan 12, 2021 · 7 comments
Open

AMP HTML documents handled via NetworkFirst strategy #70

gilbertococchi opened this issue Jan 12, 2021 · 7 comments

Comments

@gilbertococchi
Copy link

Hi amp-sw community, I've noticed that the current logic of amp-sw might cause a FCP regression on AMP sites using HTTP caching on HTML documents but also using amp-sw "as is".

Below a screenshot for amp.dev showing that page refresh requests seems treated as Network First, always downloading the HTML document from Network.
unnamed

If this is triggered by the enablement of an offline page I would suggest to switch to a CacheFirst strategy instead, maybe with a customisable max-age as safe lock or just relying on the SW versioning for caching.

FCP should improve significantly on sites using amp-sw with this strategy change.

@sebastianbenz
Copy link
Contributor

//cc @patrickkettner

@patrickkettner
Copy link

hi @gilbertococchi!

@sebastianbenz - Happy to change this on amp.dev - are you wanting to change the default behavior for amp-sw? or document why such an option should be used? would be a pretty large breaking change, but certainly doable

@kristoferbaxter
Copy link
Contributor

Changing the default would mean revisiting the decisions here: https://github.com/ampproject/amp-sw/tree/master/src/modules/document-caching

Considering the low usage of the module, this could be done with a major revision bump.

Would @sebastianbenz or @patrickkettner want to test this on AMP.dev?

@kristoferbaxter
Copy link
Contributor

Followed up in-person (over VC) and we decided to first test using a CacheFirst strategy using an override on amp.dev before changing the default.

@gilbertococchi
Copy link
Author

Would it be possible to remove default AMP HTML document caching first and respect the HTTP Caching default behaviour?
I believe that would be the safest option and then developer can intentionally handle HTML documents via their preferred Caching strategy with an ad hoc regex rule as it's possible with WorkboxJS

@jeffposnick
Copy link

Just chiming in here from a service worker perspective:

  • The CacheFirst strategy in Workbox will not update the cache "in the background" if there is a cache hit. In order words, once an HTML document is cached locally, it's effectively cached for good. I'm assuming it's not what you want for a URL that isn't uniquely revisioned. Instead, StaleWhileRevalidate would give you "use the cache if possible, but also update the cache in the background" behavior, which would be much safer to use. You might also want to explore some of the options for writing custom strategies in Workbox v6 described in this article.

  • Workbox does honor standard Cache-Control headers when it goes to the network, either in the NetworkFirst strategy or during revalidation in StaleWhileRevalidate. So if those headers are being set, you should already see them take effect.

  • Whenever you're using a service worker and potentially require a network call in order to fulfill navigation requests, using navigation preload is a best practice from a performance perspective. (Unfortunately, it's only supported in Chrome.) If you're not already using it, workbox-navigation-preload makes it pretty easy to opt-in to using it.

  • Happy to follow up with any clarifications as needed!

@kristoferbaxter
Copy link
Contributor

On these points:

  • Right now amp-sw is extended from v3 of workbox.
  • It utilizes navigation preload.
  • By default uses an extended version NetworkFirst for navigation requests to AMP documents.

We haven't upgraded to later versions of workbox due to people's availability.

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

No branches or pull requests

5 participants