-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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(gatsby-plugin-offline): fix certain resources being cached excessively #9923
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments! As with last time, were you able to validate this, and if so, what was your approach? I'd love to check it out myself!
) | ||
} | ||
}) | ||
// noop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're no longer using this, do we even need this file at all?
Additionally, we probably can remove the appending stuff, as well.
fs.appendFileSync(`public/sw.js`, swAppend) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you would ask that question - I decided to leave it because I've added some new APIs to the same file in #9907, so there doesn't seem to be much point in removing it if we're only going to add it back again later.
urlPattern: /^https?:/, | ||
handler: `networkFirst`, | ||
// Google Fonts CSS (doesn't end in .css so we need to specify it) | ||
urlPattern: /^https?:\/\/fonts\.googleapis\.com\/css/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just use a regex like in this recipe? https://gist.github.com/addyosmani/0e1cfeeccad94edc2f0985a15adefe54#cache-all-google-fonts-requests-up-to-a-maximum-of-30-cache-entries
Also from what I understand, staleWhileRevalidate will request both cache/network each time, and then update the cache on success.
It seems reasonable that something like google fonts can hit the cache first, then the network if that fails? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wrong, staleWhileRevalidate
in the Workbox config is equivalent to the CacheFirst
strategy when calling Workbox APIs directly, so the current config actually does what you've described (except for the max 30 entries).staleWhileRevalidate
checks the cache first but then checks the network in the background, even if the cache was successful. woff2
and other font files from Google (or any domain) are covered by the first regex, which matches common extensions (and now works for external resources due to the added ^https?:.*
at the start).
Also, since the linked code calls Workbox APIs directly, we'd have to move it to sw-append.js
, which means it can't be customised by the user - so I think it's best to leave it user configurable. Basically Workbox generates similar code by reading the config - sw-append.js
is for extra stuff which we can't do with the config.
const resources = [...headerResources, ...prefetchedResources] | ||
resources.forEach(resource => { | ||
// Create a prefetch link for each resource, so Workbox runtime-caches them | ||
const link = document.createElement(`link`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible that we're duplicating a prefetch here? i.e. head > link[href]
could match
<link rel="prefetch" href="/static/d/368/path---showcase-c-41-46f-ykWCDEvUZCguGaUYZsNI5k1Io.json">
and we'd create another prefetch link. Is that OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is expected behaviour - we need to "prefetch" after the SW has activated, so that Workbox can cache the responses.
link.rel = `prefetch` | ||
link.href = resource | ||
|
||
document.head.appendChild(link) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the order of these (append vs. onload) be swapped? I'd think we'd want to register the onload/remove behavior before adding it. If nothing else, seems to be more intuitive that way, even if both work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be reordered if you like - although it should work fine as-is, since the browser doesn't start fetching until it's appended. Would you like me to swap the order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, just for clarity! More of a nit than substantive, but I think it's a bit clearer.
…ively (gatsbyjs#9923) Fixes gatsbyjs#9415 This PR leverages Workbox's automatic caching when a resource is fetched, by creating a `<link>` prefetch element for each resource. This means we don't have to worry about whether the resource can be fetched with CORS or not, since the browser handles it automatically. I've also changed the runtime caching RegExps so that only paths with certain extensions are cached, but 3rd party resources with these extensions can now be cached.
Fixes #9415
This PR leverages Workbox's automatic caching when a resource is fetched, by creating a
<link>
prefetch element for each resource. This means we don't have to worry about whether the resource can be fetched with CORS or not, since the browser handles it automatically. I've also changed the runtime caching RegExps so that only paths with certain extensions are cached, but 3rd party resources with these extensions can now be cached.