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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
23599eb
Backport fixes from #9907
Nov 14, 2018
e02f9d5
minor fixes for things I copy-pasted wrong
Nov 14, 2018
fdd5844
Reorder onload / appendChild
Nov 14, 2018
14cdb95
Remove links if an error occurs
Nov 14, 2018
ff6ca9b
Add cacheFirst handler for hashed resources + make RegExps consistent
Nov 14, 2018
b25b07b
Update README
Nov 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions packages/gatsby-plugin-offline/src/gatsby-browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@ exports.onServiceWorkerActive = ({
// grab nodes from head of document
const nodes = document.querySelectorAll(`
head > script[src],
head > link[as=script],
head > link[rel=stylesheet],
head > link[href],
head > style[data-href]
`)

// get all resource URLs
const resources = [].slice
const headerResources = [].slice
.call(nodes)
.map(node => node.src || node.href || node.getAttribute(`data-href`))

Expand All @@ -40,8 +39,14 @@ exports.onServiceWorkerActive = ({
)
)

serviceWorker.active.postMessage({
api: `gatsby-runtime-cache`,
resources: [...resources, ...prefetchedResources],
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.

link.onload = link.remove
})
}
8 changes: 4 additions & 4 deletions packages/gatsby-plugin-offline/src/gatsby-node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ exports.onPostBuild = (args, pluginOptions) => {
runtimeCaching: [
{
// Add runtime caching of various page resources.
urlPattern: /\.(?:png|jpg|jpeg|webp|svg|gif|tiff|js|woff|woff2|json|css)$/,
urlPattern: /^https?:.*\.(?:png|jpg|jpeg|webp|svg|gif|tiff|js|woff|woff2|json|css)$/,
handler: `staleWhileRevalidate`,
},
{
// Use the Network First handler for external resources
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 •
edited
Loading

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.

handler: `staleWhileRevalidate`,
},
],
skipWaiting: true,
Expand Down
30 changes: 1 addition & 29 deletions packages/gatsby-plugin-offline/src/sw-append.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1 @@
/* global workbox */

self.addEventListener(`message`, event => {
const { api } = event.data
if (api === `gatsby-runtime-cache`) {
const { resources } = event.data
const cacheName = workbox.core.cacheNames.runtime

event.waitUntil(
caches.open(cacheName).then(cache =>
Promise.all(
resources.map(resource => {
let request

// Some external resources don't allow
// CORS so get an opaque response
if (resource.match(/^https?:/)) {
request = fetch(resource, { mode: `no-cors` })
} else {
request = fetch(resource)
}

return request.then(response => cache.put(resource, response))
})
)
)
)
}
})
// 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.

gatsby/packages/gatsby-plugin-offline/src/gatsby-node.js

Line 125 in e02f9d5

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.