Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AppCache fix for slow updates #248

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jampy
Copy link

@jampy jampy commented May 2, 2017

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-plugin for 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.

@NekR
Copy link
Owner

NekR commented May 3, 2017

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!

@jampy
Copy link
Author

jampy commented May 3, 2017

@NekR great! I hope you find some time very soon, as this is causing some trouble right now...

@NekR
Copy link
Owner

NekR commented May 3, 2017

@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.

@jampy
Copy link
Author

jampy commented May 4, 2017

@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.

@NekR
Copy link
Owner

NekR commented May 4, 2017

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);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jampy jampy May 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NekR ...?

@jampy
Copy link
Author

jampy commented May 28, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants