workbox-window should fire controlling events with isExternal: true

[jeffposnick]

trafficstars

In Workbox v6, we modified the behavior of most of the previous workbox-window events so that instead of having "expected" and "external" flavors (waiting, externalwaiting, etc.) we would always fire events with the expected name (waiting, etc.) and set the isExternal boolean flag accordingly.

This change did not happen for the controlling event, however, and the only time controlling will be fired is if the service worker that has taken control of the page is the same as the service worker associated with workbox-window's original registration.

We should update that so that the controlling event is also fired when it's a different service worker that's taken control, and set isExternal: true in that case. (And set isExternal: false otherwise, which also isn't being set.)

https://github.com/GoogleChrome/workbox/blob/27080cccfbc360b121083bb47e20429997051a6f/packages/workbox-window/src/Workbox.ts#L503-L517

It's arguable whether this is a bug fix or a breaking change—the documentation implies that the controlling event would be fired with isExternal: true in this scenario, so I'm erring on the side of bug fix.

CC: @philipwalton in case he has an opinion.

[denieler]

@jeffposnick Hey Jeffrey! I'm testing the newest version of Workbox and it looks like there is something working not exactly correct with this new functionality.

I have this implementation of "page reload for users":

if ('serviceWorker' in navigator) {
  const wb = new Workbox('/service-worker.js')

  const showSkipWaitingPrompt = event => {
    const result = window.confirm()
    if (result) {
      wb.addEventListener('controlling', () => {
        window.location.reload()
      })

      wb.messageSkipWaiting()
    }
  }

  wb.addEventListener('waiting', showSkipWaitingPrompt)

  wb.register()
}

and first time, when I have a new version of service worker - I decline to activate it (the new version). Then at showSkipWaitingPrompt in the event received I see isUpdate: true, isExternal: false.

Then I release a new version of the service worker again - I again decline to activate it. This time at showSkipWaitingPrompt in the event received I see isUpdate: undefined, isExternal: true.

If I repeat this process one more time - I stop receiving showSkipWaitingPrompt callbacks for the waiting event.

[jeffposnick]

Hello @denieler—how are those new releases detected? Are you calling wb.update() after each deployment of a new service worker, keeping the same page open? And which browser are you seeing this behavior with?

[chrishoage]

@jeffposnick has this functionality been released? The commit referenced (https://github.com/GoogleChrome/workbox/commit/2426f72aef51ec78c0a750c79d6615ee0aee3fe3) has not been included in any tag as far as I can tell, and npm is still at version 6.1.2

I had figured the fix for controlling not firing on an external worker had not been released yet?

[jeffposnick]

Oh, that's an excellent point—you're correct in that we need to cut a new release for that and a couple of other bug-fix PRs that have been merged recently.

CC: @tropicadri

[denieler]

@jeffposnick yes, I've tried to reproduce it with wb.update() and with

navigator.serviceWorker.getRegistrations()
    .then(serviceWorkers => {
      for (let worker of serviceWorkers) {
        worker.update()
      }
    })

However, I've tried this with the version 5.1.4 of workbox-window and it looks like the behaviour is very similar.

The first time it calls waiting event callback with isUpdate: true, the second time it calls externalwaiting event callback with isUpdate: undefined and the third time it stops calling waiting or externalwaiting.

[jeffposnick]

We're looking to get Workbox v6.1.3 out with these related changes early next week. Please give it another try then.

[denieler]

thank you @jeffposnick! definitely looking forward 🙇

[jeffposnick]

Workbox v6.1.5 (which ended up being the next release's version number) includes the changes in #2787. Hopefully things work as expected for you with that version.

[denieler]

Hey, @jeffposnick 👋 Thank you for the release you've guys made 💪 it almost fixed the issue I've reported, however it looks like there is still something wrong there happening. I don't yet have a confident way to reproduce it, but I will try and report later if will find something.

It looks like the waiting listener sometimes got "stuck" and stop being executed after multiple postponing updates for the page. Until I manually reload the page. Then it starts working again. (as I said before I do wb.update() on each react-router change)

[chandlervdw]

I'm also seeing this issue on 6.1.5 with the waiting event. It feels inconsistent as to whether the correct isExternal and isUpdate values are emitted with this event. My scenario:

  if ("serviceWorker" in navigator) {
    const wb = new Workbox('/service-worker.js');

    const showSkipWaitingPrompt = (event: WorkboxLifecycleWaitingEvent) => {
      if (event.isExternal) {
        notification({
          text: "App Update Available",
          onClick: () => {
            wb.addEventListener("controlling", e => {
              window.location.reload();
            });
            wb.messageSkipWaiting();
          }
        });
      }
    };


    wb.addEventListener("waiting", showSkipWaitingPrompt);

    wb.register();
  }

It’s hard to reproduce but it doesn't seem to matter if the tab is in the background or not and generally just feels inconsistent. I feel like I'm super close with this but this really feels like an issue with workbox-window’s waiting event.