-
Notifications
You must be signed in to change notification settings - Fork 9
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
Workbox #162
base: main
Are you sure you want to change the base?
Workbox #162
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.
Woohoo! Nice job @calebeby! 👏
I've got some feedback but this is a great start! 😃
sw.js
Outdated
// Cache CSS and JS files | ||
workbox.routing.registerRoute( | ||
/\.(?:js|css)$/, | ||
workbox.strategies.staleWhileRevalidate({ |
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.
For static assets, we should use a cacheFirst()
strategy instead. 😉
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.
Can you explain why? My reasoning for using staleWhileRevalidate
is that then it would update the file in the background (for example if the css file is updated)
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.
@calebeby You've convinced me. I did some more digging and it looks like using staleWhileRevalidate
is a common way to handle CSS & JS that isn't precached. Thanks for double-checking! 😉
sw.js
Outdated
* Precache resources | ||
*/ | ||
staticCache.addToCacheList({ | ||
unrevisionedFiles: ['/', '/404/', '/error/'] |
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.
It looks like the service worker was precaching resources before. Let's make sure the new service worker does the same. :)
Here are some docs about precaching files: https://developers.google.com/web/tools/workbox/guides/precache-files/
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.
sw.js
Outdated
plugins: [ | ||
new workbox.expiration.Plugin({ | ||
maxEntries: 60, | ||
maxAgeSeconds: 30 * 24 * 60 * 60 // 30 Days |
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.
Curious how you landed at a max entry amount of 60 and a max age of 30 days? It feels like a guessing game without something to lean on but was wondering if you had referenced a best practice you found. :)
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.
Wondering if @grigs has any thoughts on this for the images cache? :)
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 used the example from https://developers.google.com/web/tools/workbox/guides/common-recipes. I think that 30 days is reasonable, but maybe we could remove the maxEntries
?
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.
Sounds good. Keeping what you have works for me. 😉
sw.js
Outdated
maxEntries: 30 | ||
}), | ||
new workbox.cacheableResponse.Plugin({ | ||
statuses: [0, 200] |
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'd rather we try not to cache opaque responses (the 0
status). Is there a way to opt-in to CORS mode for the font requests?
Not sure if this helps but just in case: https://developers.google.com/web/tools/workbox/guides/handle-third-party-requests
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.
Oh, nice! I just checked and the fonts are already opting into CORS mode via the crossorigin
attribute. 😉
<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Source+Sans+Pro:400,600,700" crossorigin>
You should be fine by just removing the 0
status from the array. 👍
_layouts/blank.html
Outdated
@@ -6,7 +6,7 @@ | |||
{% include svg-icons.html %} | |||
<script> | |||
if ('serviceWorker' in navigator) { | |||
navigator.serviceWorker.register('{{ site.url }}/sw.js'); | |||
navigator.serviceWorker.register('/sw.js'); |
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.
Can we also add a catch()
just in case the service worker fails to register? 😬
Something like:
navigator.serviceWorker
.register()
.catch(error => {
console.error('Service worker registration failed:', error);
});
@@ -6,7 +6,7 @@ | |||
{% include svg-icons.html %} | |||
<script> | |||
if ('serviceWorker' in navigator) { |
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.
Since we are making changes, can me add a friendly console warning message if service workers are not supported?
Something like:
if ('serviceWorker' in navigator) {
// ...
} else {
console.warn('Service workers are not supported by this browser. :(');
}
); | ||
|
||
// Make this always match package.json version | ||
const version = '0.1.3'; | ||
const cacheName = `defaultCache_${version}`; |
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.
Oh, one more thing I noticed! I ran the site locally, and the defaultCache_*
cache is sticking around. 🙈
We'll want to purge the out-of-date caches as well. Workbox does not handle this for us. Instead, it is our responsibility to remove out-of-date caches.
In another project, I set up a two-level version cache naming system. Here are some snippets in case it helps (the module.exports
may or may not apply), feel free to steal anything that makes sense:
// sw/caches.js
/**
* This sets up a two-level version cache naming system.
* Edit the GLOBAL_VERSION to purge all caches.
* Edit the individual cache version to purge the specific cache.
*/
const GLOBAL_VERSION = 1;
const CACHES = {
PRECACHE: `pwastats-precache-v${GLOBAL_VERSION + 0}`,
FONTS: `pwastats-fonts-v${GLOBAL_VERSION + 1}`,
STATIC_ASSETS: `pwastats-static-assets-v${GLOBAL_VERSION + 0}`,
IMAGES: `pwastats-images-v${GLOBAL_VERSION + 0}`
};
/**
* This allows the CACHES to be available both in the
* workbox.config.js as well as within the service worker.
*/
try {
module.exports = CACHES;
} catch (e) {
// Do nothing
// For when CACHES is used within service worker scope.
}
// sw/sw-delete-caches.js
importScripts('/sw/caches.js');
const validCacheList = Object.values(CACHES);
/**
* Checks the cache name in question against the valid list of caches
* @param {String} cacheName The name of the cache in question
* @returns {Boolean} Is the `cacheName` valid?
*/
const cacheIsValid = cacheName =>
validCacheList.some(validCacheName =>
cacheName.includes(validCacheName)
);
/**
* Delete out-of-date caches
*/
self.addEventListener('activate', event => {
event.waitUntil(
caches.keys().then(cacheNameList => Promise.all(
cacheNameList.map(cacheName => {
if (!cacheIsValid(cacheName)) {
console.info(`SW :: Deleting cache "${cacheName}"`);
return caches.delete(cacheName);
}
})
))
);
});
// sw.js
importScripts("https://storage.googleapis.com/workbox-cdn/releases/3.2.0/workbox-sw.js");
importScripts("/sw/sw-delete-caches.js");
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.
Thanks for making all of those udpates @calebeby! 😄
A few more inline comments. Also, can you look into skipWaiting()
? I think we want to set it up so that the service worker can update and control the web page immediately.
https://developers.google.com/web/tools/workbox/modules/workbox-sw#skip_waiting_and_clients_claim
sw.js
Outdated
router.registerRoutes({ | ||
routes: [assetRoute, cdnAssetRoute, navRoute] | ||
}); | ||
workbox.precaching.precache(['/', '/404/', '/error/']); |
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.
@calebeby Hmm...I'm getting a Workbox warning. 🤔
The following precache entries might not be revisioned:
- "/"
- "/404/"
- "/error/"You can learn more about why this might be a problem here: https://developers.google.com/web/tools/workbox/modules/workbox-precaching
Maybe you can look into using the Workbox CLI (maybe as an npm task?) to automate this for us?
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 I do that in this PR or another?
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.
@calebeby I think it should be a part of this PR.
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.
On it!
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.
That was very overly complicated, but now it works ✔️
As I understand it, this is the behavior we want for html files (which I implemented):
prefer network
fall back to cache for that page (whether it is precached or user visited it before)
fall back to displaying precached 404.html page
Workbox really doesn't want to do this (especially the 404 fallback part), so I had to implement a custom handler
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.
Perhaps late but I found this. I'll share it still in case it helps: https://stackoverflow.com/questions/50762234/configure-workbox-to-use-cached-response-when-network-response-is-a-404
As I understand it, this is the behavior we want for html files (which I implemented):
This makes sense to me.
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.
That does not work with precaching (workbox has a whole separate cache for precaching that works differently, and the paths will not match)
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.
Also, if you have a network connection, and the server responds with a 404, you can just respond with the 404 page that came from the server, instead of doing caching things with that
handler: new runtimeCaching.NetworkOnly() | ||
}); | ||
const isCacheValid = cacheName => | ||
Object.values(CACHES).includes(cacheName) || cacheName.startsWith('workbox-'); |
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.
@calebeby Sorry, I misled you. 🙈 Workbox does handle any caches it creates (the ones that start with workbox-
). We have to handle any caches we create on our own (e.g. css-js
, google-fonts
, images
).
It should be fine w/o the || cacheName.startsWith('workbox-')
here. 😬
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 wanted to have a whitelist of caches that would be allowed, the rest would be deleted. I allowed the specific ones that we had named, and any starting with workbox-
. Any other cache was deleted
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.
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.
Got it, cool. Thanks for explaining! 👍
router.setDefaultHandler({ | ||
handler: new runtimeCaching.NetworkOnly() | ||
}); | ||
const isCacheValid = cacheName => |
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.
Thanks for renaming this method name. 👍😉
_layouts/blank.html
Outdated
@@ -6,7 +6,9 @@ | |||
{% include svg-icons.html %} | |||
<script> | |||
if ('serviceWorker' in navigator) { | |||
navigator.serviceWorker.register('/sw.js'); | |||
navigator.serviceWorker.register('/sw.js').catch(e => console.error("Problem installing service worker:", e)); |
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.
Nit: Single quote for consistency? :)
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.
✔️
@calebeby One more thing! I was seeing this in the DevTools console when offline. I'm assuming this is because we are using
|
Yep, I don't think it's an issue if it tries to request a file from the network while offline |
@gerardo-rodriguez I think I addressed all of your feedback, when you get a chance can you look at the changes? |
Replaces service worker plugins with workbox
Closes #161