workbox
workbox copied to clipboard
Listen statechange on waiting as well
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.
Coverage remained the same at 79.314% when pulling f4b6bb6e298f070b40a0bc54ab94d5a18aed2915 on azizhk:statechange_waiting into ae80e3958c79cb93f4c21bca63cd6e81f7878536 on GoogleChrome:master.
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.
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.
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.
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
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.
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.