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

fix(gatsby-plugin-offline): fix certain resources being cached excessively #9923

Merged
merged 6 commits into from
Nov 14, 2018
Merged

fix(gatsby-plugin-offline): fix certain resources being cached excessively #9923

merged 6 commits into from
Nov 14, 2018

Conversation

vtenfys
Copy link
Contributor

@vtenfys vtenfys commented Nov 14, 2018

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.

@vtenfys vtenfys requested a review from pieh November 14, 2018 14:18
Copy link
Contributor

@DSchau DSchau left a 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
Copy link
Contributor

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)

Copy link
Contributor Author

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/,
Copy link
Contributor

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?

Copy link
Contributor Author

@vtenfys vtenfys Nov 14, 2018

Choose a reason for hiding this comment

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

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). I was wrong, 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`)
Copy link
Contributor

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?

Copy link
Contributor Author

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@DSchau DSchau merged commit 7c826a1 into gatsbyjs:master Nov 14, 2018
@vtenfys vtenfys removed the request for review from pieh November 14, 2018 20:23
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
…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.
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.

2 participants