GoogleChrome / workbox

📦 Workbox: JavaScript libraries for Progressive Web Apps
https://developers.google.com/web/tools/workbox/
MIT License
12.27k stars 805 forks source link

Remove "external" flavor of workbox-window events #2031

Closed CaptainCodeman closed 4 years ago

CaptainCodeman commented 5 years ago

Library Affected: workbox-window

Browser & Platform: Chrome 72

Issue or Feature Request Description: If the service worker update is triggered from within the app using ServiceWorkerRegistration.update() instead of automatically at page load then the workbox-window events don't work as they normally do, breaking the update mechanism which otherwise works as expected when used with normal app re-launch.

This would be useful for long-running devices where the trigger for a version check can be pushed out (e.g. by the device subscribing to firebase, having a notification sent etc...)

Will try to create a simplified reproduction.

philipwalton commented 5 years ago

Hmmm, yes, please try to create a repro, as I wouldn't expect this to be the case. We have an .update() call in one of our workbox-window tests, so I know (at least in some cases) it's working.

jeffposnick commented 5 years ago

See also: #2130

alex-stumpfl commented 4 years ago

The problem seems to be, that the behaviour in case of ServiceWorkerRegistration.update() is not deterministic. Sometimes it works as expected (resulting in a 'waiting' event) while in other cases an 'externalwaiting' event is triggered instead. Couldn't reproduce that consistently (so it's pretty hard to test), but it primarily happens, when the page was offline for a while (by devtools or server shutdown) and became online again.

daffinm commented 4 years ago

Browser and platform

"Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/78.0.3904.70 Safari/537.36"

I am also being bitten by this issue when using workbox-window.update() and have written it up in this stackoverflow question which I note has a similar title to this issue.

The test steps with my app are always the same and yet sometimes the external versions of the lifecycle events get triggered.

  1. Open the app in a new tab and ensure that only one instance of the app is running.
  2. Make changes to the app that result in updates to the service worker definition.
  3. Click a button in the app that causes workbox-window.update() to run.

Most of the time the waiting event handler is triggered by the update() call, in which case the update process can complete by offering an 'update available - install now?' prompt in the activated event handler. If the user accepts then a SKIP_WAITING message can be sent to the service worker so that the app can be reloaded in the activated event handler to complete the update process.

But sometimes, for no apparent reason, the externalwaiting event is triggered instead. When this happens the SKIP_WAITING message sent by the externalwaiting event handler seems to disappear into thin air and the new service worker is left in a waiting state observable via Chrome Dev Tools. The update process therefore never completes.

Happy to provide example code if this will help.

daffinm commented 4 years ago

Further update iteration testing in Chrome 78.0.3904.70 (Official Build) (64-bit). Conclusion: The logic for distinguishing between and internal and external service worker update seems 'temperamental'.

Test setup

Test 1 Press the 'Check for Updates' button in B.

Test 2 Press the 'Check for Updates' button in B.

Test 3 Press the 'Check for Updates' button in B.

Two observations

  1. The logic to distinguish 'internal' from 'external' lifecycle events seems temperamental. I have also seen it flick between internal and external when running update iteration tests in the same tab with no other open tabs over successive test iterations. (I can find no consistent way of reproducing this. Last time it happened was after I had left the app running in a single tab for a few hours before running a new update iteration test.)
  2. Messages sent from the externalwaiting handler using workbox-window.messageSW() are received by the active, old service worker, not the waiting service worker. So it remains in a waiting state. What does one do about this? (If you want proof of this see the logs in this stackoverflow question.
jeffposnick commented 4 years ago

@novaknole recently brought this up in https://github.com/GoogleChrome/workbox/issues/2165 as well.

CC: @philipwalton for follow-up.

jeffposnick commented 4 years ago

In retrospect, I wonder if the externalwaiting vs. waiting distinction exposed in workbox-window has ended up being a net-positive for developers.

Perhaps moving forward, coalescing both into a single (presumably new) event would make sense? The separate events could still be maintained for backwards compatibility.

Alternatively, maybe we need to update our recipes to add in listeners for both waiting and externalwaiting, where we execute the same event handler for both.

novaknole commented 4 years ago

@philipwalton @jeffposnick

Looks like After some debugging, I have found something as @daffinm found out and more.

Code i call when my NOT real navigation happens:

Vue.prototype.$checkForUpdate = () => {
    if(firstTime) {
        firstTime = false;
        return;
    }
    navigator.serviceWorker.ready.then((reg) => {
        reg.update();
        // onNewServiceWorker(reg);  
    })
}

Assumption After yesterday, I tried to implement workbox-window plugin myself. pure SW implementation. Looks like new service worker would activate immediatelly when calling reg.update() if browser wound find a new one. That's when there's no navigation and we just simply click on a button to check if there's an update. So, then i figured out that this only happens when user goes to the webpage for the first time ever and doesn't do anything yet. meanwhile, new service worker gets on the server. now, user clicks on a button check if there's update and we call reg.update() . After clicking, new service worker starts installing and gets activated. The only time new service worker wouldn't get activated after calling reg.update() is if user goes to the website for the first time and then refreshes the page before clicking on a button.

So, I don't know how exactly in a nutshell clientsClaim() solves that(Any idea ? ), but after adding it, my pure SW implementation started working all the time.

Then, I switched back to workbox-window plugin. I also added clientsClaim in SW. and workbox also didn't activate new service worker immediatelly which is good. but now, as @daffinm said, sometimes it calls waiting and sometimes externalwaiting. and this makes a problem as skipWaiting() doesn't work anymore because it goes for old service worker. Any suggestions?

What doesn't work:

A) I opened the first tab. it installed SW. all good. Then I changed service worker code, deployed it. I try to open another tab and go to my website. the first tab is still open. After opening my site in second tab, new service worker gets installed. and it goes to waiting state. in my first tab, it logged externalwaiting state. on second tab, it logged waiting state. Now, it shows my button on both tabs and on clicking it, it should activate new service worker.

B) If i only have 1 tab and if i listen the route change in my SPA and call reg.update(), waiting event doesn't get called. but externalwaiting event gets called. which stops my app from working because if i click update button now, even though skipWaiting gets called, it doesn't activate new SW.

novaknole commented 4 years ago

@jeffposnick @philipwalton hey guys, when do you think you might give me some heads up or look through what I commented?

daffinm commented 4 years ago

Glad to see that this is getting some attention now.

Perhaps moving forward, coalescing both into a single (presumably new) event would make sense? The separate events could still be maintained for backwards compatibility.

This seems like the simplest idea. It feels like the waiting/externalwaiting distinction is a bit of an over-refinement.

Alternatively, maybe we need to update our recipes to add in listeners for both waiting and externalwaiting, where we execute the same event handler for both.

This is what I tried first -- calling the same method from handlers for both types of waiting event. The problem is that the waiting service worker cannot be contacted from the externalwaiting handler code. The skipwaiting message is received by the older, active service worker. If this was fixed then you could indeed just update your docs, which might be less work.

philipwalton commented 4 years ago

Hey all, just catching up here now (sorry, for some reason I wasn't seeing updates to this issue)!

From reading all the responses here, it sounds like the main actual pain point is that wb.messageSW() always messages the instance's "own" service worker rather than the "external" service worker—even in cases where the external* events are being dispatched.

The thinking there was that, when there are two service workers in play, workbox-window cannot know which one you intend to message, so it always favors its "own" service worker.

There is a solution, though. The workbox-window package does include a standalone messageSW() function that can be used to message any service worker that you have a reference to. Here's an example:

import {Workbox, messageSW} from 'workbox-window';

async function main() {
  const wb = new Workbox('/sw.js');

  wb.addEventListener('externalwaiting', (event) => {
    // Sends the message to the external service worker.
    messageSW(event.sw, {type: 'SKIP_WAITING'});
  });

  await wb.register();
}
main();

Does this address the issues brought up here?


On the question of whether or not we should have external versions of these events at all, before making any changes to the API, I'd like to better understand why folks here want to message the external, waiting service worker. Is it just to call skipWaiting() and then reload? Or are folks wanting to skip waiting and then keep using the current page without reloading?

I ask because most cases where the externalwaiting event fires involve two tabs open, where one of them will get the regular waiting event, and so the code in this recipe would work.

The only way externalwaiting could be fired in a single-tab scenario is if the app is calling update() long after first calling register(), and a new version of the SW has been deployed at some point in between. In such cases I can see why a developer would want to conditionally send a SKIP_WAITING message and then reload, but in all other cases I would not recommend building functionality that tries to communicate with an external SW (since that SW will be a future version of your app and you can't possibly anticipate what changes you might have made in it).

But perhaps the vast majority of people using the messageSW() method are just using it to call skipWaiting(), in which case maybe all the precautions I've put in are more trouble than they're worth.

daffinm commented 4 years ago

Hi @philipwalton. I don't know what the vast majority are doing. I only know what I am doing. And (as usual) I am doing my best to keep things simple, and so I may have an overly simplistic take on all this.

I am not sure I understand the distinction between waiting and externalwaiting... Here's my current understanding.

When I tested all this originally (see message above for summary) the listeners for the waiting and externalwaiting events seemed to get triggered randomly - at least I could see no pattern. Although I could more reliably trigger externalwaiting in the two tab sceanario it did not always occur, and I also saw this event in single tab mode, sometimes when the app had been left idle for no more than a few minutes (although this did seem to happen more often when it had been idle for longer).

In either case (single or multiple tabs) I would still like to be able to inform the user via a choice dialog of some kind:

"An update is available for this app." [Use it now?] [Later...]

If 'use it now' is clicked then a SKIP_WAITING message is sent to the waiting service worker. I would then listen for this service worker to activate and reload the app window so that the front and back end are in sync (v2 to v2). Otherwise the user continues with their current version of the front end using the correct version of the back end (v1 to v1), and they get the v2 service worker and v2 front end next time they reload the app.

I don't see why it matters if the service worker update occurred because of an action occurring in another tab, or after a long delay in one tab, or because of a periodic check for an update issued by the browser in the background.

As for your solution using existing code, I will test it and get back to you here. Thanks for explaining it.

johnrobertcobbold commented 4 years ago

Hello all,

I will join this conversation that I am happy to come across now as I actually have been struggling with this issue since I started implementing registration.update() a couple of hours ago. My goal is to also display our 'update app' button (which when clicked sends the SKIP_WAITING message to the sw using wb.messageSW({type: 'SKIP_WAITING'}); and then reloads the page on controlling or controllerchange event) when registration.update() detects a change. Everything works fine when 'waiting' is fired but not when 'externalwaiting' is.

@philipwalton your solution above using messageSW(event.sw, {type: 'SKIP_WAITING'}); works and the new service worker kicks in. One problem though: neither thecontrolling nor the controllerchange events get fired, even when I tried using ev.sw.addEventListener('controlling', (event) => {window.location.reload();} so our prompt stays there and the page does not reload. For now, in the case of externalwaiting, I switched to ev.sw.addEventListener('statechange',... which works, although I lost track if that makes sense or not, although it seems to work..

As for when and how externalwaiting gets fired since I am only using one tab? Every time the service worker gets installed, the first call to registration.update() after the SW was changed fires an `externalwaiting'.

philipwalton commented 4 years ago

@daffinm

I am not sure I understand the distinction between waiting and externalwaiting... Here's my current understanding.

Your understanding is correct about the general service worker update cycle. With regards to the difference between waiting and externalwaiting in workbox-window, here's a breakdown of the logic:

An "external" version of each lifecycle event will be triggered if any of the following are true:

That last point is a bit of a hack, but currently the service worker API does not provide any signal to the developer as to whether or not an invocation of update() or register() actually found an update, so this is the best we can do.

Does that help explain why you're sometimes seeing waiting and other times seeing externalwaiting?

I don't see why it matters if the service worker update occurred because of an action occurring in another tab, or after a long delay in one tab, or because of a periodic check for an update issued by the browser in the background.

In your specific case, I don't think it does matter. If all you're planning to do is prompt the user to refresh any time there's an update, then there's no meaningful difference between the events. The difference is intended for cases where a developer only wants to prompt a user for an update in the case of an "external" (a.k.a. unexpected) service worker being installed.

That being said, I discussed with @jeffposnick last week and we decided that in v6 we'll likely remove the "external" versions of each event and instead put an isExternal property on the event object. We think that'll make it easier for developers to handle the most common use cases (yours being one of them) while still giving them all the information they currently have today.

What do you think?

philipwalton commented 4 years ago

@goyavo

One problem though: neither the controlling nor the controllerchange events get fired, even when I tried using ev.sw.addEventListener('controlling', (event) => {window.location.reload();} so our prompt stays there and the page does not reload.

I'd have to see a repo to know for sure, but in my experience, the most common reason a service worker would fail to activate (and thus take control) is that the currently active service worker has an extend lifetime promise that hasn't yet resolved.

For example, if you have the following fetch event listener in your active service worker, then calling skipWaiting() will not cause it to become redundant because the promise passed to event.waitUntil() will never resolve:

addEventListener('fetch', (event) => {
  // Respond by fetching the request.
  event.respondWith(fetch(event.request));

  // Call `waitUntil()` with a promise that will never resolve.
  event.waitUntil(new Promise(() => {});
});

Could that be the case in your code?

daffinm commented 4 years ago

@philipwalton. It sort of explains. (But it also tells me that I have not got my head fully wrapped around this thing yet...)

I think that sounds like the ideal solution. What's the ETA for V6?

plandem commented 4 years ago

@philipwalton

messageSW(event.sw, {type: 'SKIP_WAITING'});

it will be same as

messageSW(registration.waiting, {type: 'SKIP_WAITING'})

right?

jeffposnick commented 4 years ago

messageSW(registration.waiting, {type: 'SKIP_WAITING'}) is, I think, a better approach, as it ensures that you're always sending the SKIP_WAITING message to a service worker that's actually waiting.

That's the logic that messageSkipWaiting() uses in v6: https://github.com/GoogleChrome/workbox/pull/2489/files#diff-3c13b193f7b64e2ef506486a17173077R291-R295

I'd recommend just giving the v6 prerelease a try.

plandem commented 4 years ago

@jeffposnick I would like to give a try for v6 pre-release, but afraid to spend extra time on figuring out what its not working - is it SW in general, our implementation or just library. I already spent time fighting with some issues here and there and everything is kinda working right now (I hope).

Is v6 consistent about controlling event? Because v5 is not and I have to use

navigator.serviceWorker.addEventListener('controllerchange'

to make sure it works as supposed.

task is simple - doesnt matter how many tabs were opened (one tab can be first one that installed SW and claimed control), any update to SW should inform user about updates and on confirmation all tabs should be reloaded. In our case it's dead simple logic - SW is updated each app rebuild due to precache, so we should not have SW older/newer than app. And with controlling that behaviour was not possible to achieve, its not firing for one or two cases.

jeffposnick commented 3 years ago

@plandem, I'm just seeing your last message now. #2786 tracks making sure that controlling is fired consistently in v6, with isExternal used to distinguish between the two cases (for those who care).