AppCache fix for slow updates #248
Conversation
…i when updates take longer than 5 seconds
|
This is most likely good to merge, I just don't have time right now to make a new release, but I'll do eventually. Thanks for the PR! |
|
@NekR great! I hope you find some time very soon, as this is causing some trouble right now... |
|
@jampy I'll release it this week. Also if Apple isn't going to ship ServiceWorker this year then I probably should improve AppCache events and make them all work correctly. |
|
@NekR It's unlikely that we'll see SW on Safari soon, unfortunately. :-( Also note that Service Workers are still unsupported on IE/Edge, too. |
|
Edge is shipping this year. Apple actively participates in SW specification, you have to look between lines ;-) IE.. well, I don't care very much. |
| @@ -90,9 +90,6 @@ | |||
| clearInterval(downloadingInterval); | |||
| downloadingInterval = null; | |||
| } | |||
|
|
|||
| applicationCache.removeEventListener('downloading', onDownloadingEvent); | |||
| applicationCache.removeEventListener('progress', onDownloadingEvent); | |||
There was a problem hiding this comment.
Okay, problem with removing this is that progress happens multiple times, i.e. it may happen once per file. So removing this cleanup makes onUpdating event fire very often.
There was a problem hiding this comment.
Also, I'm probably wrong about firing it multiple times because of updatingFired check. But this check will prevent onUpdating event to fire when autoUpdate happens. In other words, it's more complicated than just removing those lines.
There was a problem hiding this comment.
Isn't it by design (and desired) that progress happens multiple times? Even with the current code those events come in the same way in the first 5 seconds. I don't see the problem, sorry.
What about just keeping the updateready event? After all that's the single most important one to get updates working instantly AFAIK. That would still remain a simple fix that could be released soon.
As for a more complete fix I think it would be best if the event handlers (perhaps those outside of the iframe) can deal with various event "frequencies". After all we learned that browser behaviour is somewhat unpredictable and nobody can guarantee what events come in in what order...
|
If anyone is interested, I've published a patched version of this module (based on v4.8.1 plus the changes discussed here) - simply because this discussion is taking too long for our real-world problem: https://www.npmjs.com/package/offline-plugin-patched It will hopefully become obsolete soon once this PR is accepted or another solution is available. |
related to #241 and #210
This patch simply prevents unregistering of the Application Cache event handlers after 5 seconds - only the timer is still being stopped after 5 seconds (the timer is used as a workaround should events not work).
This fixes
offline-pluginfor iPhone/iPad/Safari (tested) and IE/Edge (probably) when 5 seconds are not enough to check for updates and download assets.IMHO this shouldn't break anything as the timer-workaround will behave like before. The 5-seconds problem will continue to exist for situations where the timer is really needed (when events don't work due to browser bugs).
I don't know which browsers really need the timer hack, but events seem to work fine on iOS/Webkit and also IE10 & Edge, where this patch makes a huge difference, because application get the chance to update immediately.