New advanced recipe for service workers may not be correct

[dfabulich]

Page Affected: https://developers.google.com/web/tools/workbox/guides/advanced-recipes

Not deployed yet; new in fff418b

What needs to be done?

The code suggested to add to the service worker looks like this:

self.addEventListener('message', (event) => {
  if (!event.data){
    return;
  }

  switch (event.data) {
    case 'force-activate':
      self.skipWaiting();
      self.clients.claim();
      self.clients.matchAll().then((clients) => {
        clients.forEach((client) => client.postMessage('reload-window'));
      });
      break;
    default:
      // NOOP
      break;
  }
});

The problem with this approach is that it can post a message to the clients before the new service worker has activated. skipWaiting initiates activation, but activation can take a while, and we need to wait to reload the page until after activation is finished.

The guide also links to my article on this topic https://redfin.engineering/how-to-fix-the-refresh-button-when-using-service-workers-a8e27af6df68 (I'm quite grateful for that!)

There, I recommend reloading the page during the oncontrollerchange event. That way, the page-client reloads itself only after activation is completely done.

navigator.serviceWorker.addEventListener('controllerchange',
  function() { window.location.reload(); }
);

I think that's the best way to ensure that reloading happens post activation.

[dfabulich]

I mentioned in a comment on GoogleChrome/workbox#1120 that I think it would be better for this guide to recommend a 'prolyfill' (a forward-looking polyfill for behavior that may one day be standardized).

I've posted a suggested prolyfill in a comment to w3c/ServiceWorker#1222 and I think it would be better to just standardize on that.

[gauntface]

@dfabulich I originally had the controllerchange, the issue there is gets in a endless loop if you use update on reload which I think some of the twitter bootstrap developers hit at one point.

Generally I think Workbox can provide a library to make this way easier I'm also fine with replacing this advice with something simpler should it exist (i.e. a polyfill that could hopefully be loaded via babel-preset-env.) but I would like to provide some advice for developers that applies today.

Would replacing the service worker code with, appease your activation concerns?

self.addEventListener('activate', (event) => {
  event.waitUntil(
    self.clients.claim()
    .then(() => {
      self.clients.forEach((client) => client.postMessage('reload-window'));
    })
  );
});

[gauntface]

Ignore that it doesn't work - I'll just file an issue on devtools and use controller change

[gauntface]

cc @jeffposnick @dfabulich I filed a PR was hoping to get feedback before it got merged so apologies for that. Let me know if the new stuff is still not ok with y'all

[dfabulich]

I think what you have is OK, but I'd definitely still prefer/recommend using a prolyfill to make the code simpler. Some day, the code should be 100% client-side, and should be very brief:

function showRefreshUI(registration) {
  // TODO real implementation
  if (confirm("Refresh now?") && registration.waiting) registration.waiting.skipWaiting();
}

navigator.serviceWorker.addEventListener('controllerchange', function() {
  window.location.reload();
});

navigator.serviceWorker.waiting.then(showRefreshUI);

I've filed PR #5508 on this.

[gauntface]

I'm going to close this for now.

If the proposals gain traction I'm happy to switch over to them.