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

Workbox #162

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Workbox #162

wants to merge 14 commits into from

Conversation

calebeby
Copy link
Member

Replaces service worker plugins with workbox

Closes #161

@calebeby calebeby changed the title Feature/workbox Workbox Jul 11, 2018
Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a 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({
Copy link
Member

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. 😉

Copy link
Member Author

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)

Copy link
Member

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/']
Copy link
Member

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/

Copy link
Member

@gerardo-rodriguez gerardo-rodriguez Jul 12, 2018

Choose a reason for hiding this comment

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

You can see the difference in action by going offline via the DevTools and trying to hit the 404 page:

screen shot 2018-07-12 at 4 10 09 pm

sw.js Outdated
plugins: [
new workbox.expiration.Plugin({
maxEntries: 60,
maxAgeSeconds: 30 * 24 * 60 * 60 // 30 Days
Copy link
Member

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. :)

Copy link
Member

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? :)

Copy link
Member Author

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?

Copy link
Member

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]
Copy link
Member

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

Copy link
Member

@gerardo-rodriguez gerardo-rodriguez Jul 12, 2018

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. 👍

@@ -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');
Copy link
Member

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) {
Copy link
Member

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}`;
Copy link
Member

@gerardo-rodriguez gerardo-rodriguez Jul 12, 2018

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. 🙈

screen shot 2018-07-12 at 3 44 33 pm

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");

Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a 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/']);
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

On it!

Copy link
Member Author

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 ☹️

Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member Author

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-');
Copy link
Member

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. 😬

Copy link
Member Author

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

Copy link
Member Author

@calebeby calebeby Jul 17, 2018

Choose a reason for hiding this comment

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

If I didn't whitelist caches starting with workbox-, all the workbox caches would be deleted

screen shot 2018-07-17 at 10 05 09 am

Copy link
Member

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 =>
Copy link
Member

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. 👍😉

@@ -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));
Copy link
Member

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? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

✔️

@gerardo-rodriguez
Copy link
Member

@calebeby One more thing! I was seeing this in the DevTools console when offline. I'm assuming this is because we are using staleWhileRevalidate, therefore, I don't think it's an issue. But wanted to call it out in case you saw any different. 😉

[workbox] Network request for '/main.css' threw an error. TypeError: Failed to fetch

@calebeby
Copy link
Member Author

Yep, I don't think it's an issue if it tries to request a file from the network while offline

@calebeby
Copy link
Member Author

@gerardo-rodriguez I think I addressed all of your feedback, when you get a chance can you look at the changes?

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