workbox icon indicating copy to clipboard operation
workbox copied to clipboard

Listen statechange on waiting as well

Open azizhk opened this issue 4 years ago • 7 comments

Prior to filing a PR, please: Fixes #2199 R: @jeffposnick @philipwalton

Description of what's changed/fixed. Changes as described in https://github.com/GoogleChrome/workbox/issues/2199#issuecomment-526790836 Writing tests with my scenario.

azizhk avatar Aug 31 '19 10:08 azizhk

Coverage Status

Coverage remained the same at 79.314% when pulling f4b6bb6e298f070b40a0bc54ab94d5a18aed2915 on azizhk:statechange_waiting into ae80e3958c79cb93f4c21bca63cd6e81f7878536 on GoogleChrome:master.

coveralls avatar Aug 31 '19 10:08 coveralls

So I realized that our problem would also occur for installing service workers as well. I wrote some tests but I am having some problems getting them working on all browsers.

azizhk avatar Sep 02 '19 11:09 azizhk

Hmmm, regarding triggering an installing event with wasInstallingBeforeRegiser:true, I'm not sure how much that makes sense. I agree it can happen in the case in your example in #2199, but in order for it to happen you have to delay calling register(), and in general I'm not sure if that's a pattern we want to promote or support with workbox-window.

Can you elaborate as to why you want to do this?

@jeffposnick what are your thoughts here? Are you aware of reasons (for or against) delaying the register() call? If we choose to support this use case then we'd have to make a lot of changes to the Workbox class, because at the moment it doesn't add any lifecycle listeners until register() is called, which means updates triggered by the browser's soft update checks would be missed.

philipwalton avatar Sep 02 '19 18:09 philipwalton

Well I don't necessarily need to trigger installing. I just added it to keep parity with waiting.

But I would like to add statechange listener on the installingSW. This is because the user/developer can call register at any point. (After requestIdleCallBack, etc.) And this can cause race conditions with browser's soft update.

I'm ok with not adding any lifecycle listeners until register() is called. It's just that documentation needs to mention that one can't expect all events to be triggered in sequence. They can expect workbox to trigger "waiting" & "activated" events (as that covers majority use case) But for "installing" & "installed" they should check registration.installing as well as listen for these events.

azizhk avatar Sep 03 '19 07:09 azizhk

Also I've add statechange listeners on both waiting & installing SWs if both exist. I think we should add them on all this._ownSWs that you are adding in #2211 diff

azizhk avatar Sep 03 '19 08:09 azizhk

Hey @azizhk after discussing with @jeffposnick I've filed #2216 to track support for issues and API changes surrounding conditional registration.

Can we keep this PR focused on the original problem where service workers that were waiting before register do not receive any additional lifecycle events?

Then we can discuss in #2216 how best to update workbox-window to handle conditional/deferred registration.

philipwalton avatar Sep 03 '19 19:09 philipwalton

Sure, I've reverted the code. But am not sure why tests are failing on "Safari Stable" I'll also isolate my other failing test in a different PR.

azizhk avatar Sep 04 '19 01:09 azizhk