-
Notifications
You must be signed in to change notification settings - Fork 312
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
should update on navigation or the subsequent updatefound event be delayed until document DOM is loaded? #788
Comments
In this gecko bug I am modifying our code to delay update until a controlled document reaches DOMContentLoaded. |
Presumably chrome is delaying for such a long time so that it the update does not regression load times. |
Yes Chrome triggers update after a one second delay after load completes, so it doesn't contend with the network for loading the page. |
@annevk what'd be a good way to spec this requirement? Setting a fixed amount of delay in the spec doesn't seem like a good idea. Also, the target resource can be a worker, a shared worker as well as a document, so no unified concept can be referenced to get the right timing. Any idea? |
Depends on the semantics you want I suppose. (Note that workers cannot be navigated so I'm not sure how the same problem applies to them.) |
Soft Update is triggered for all the non-subresource requests (whose destination is not " |
@mattto What does chrome do if the client has been closed when the update timer triggers? Do you go ahead with the update or cancel the update? |
We continue with the update, it's scheduled on the SW entity itself rather than attached to the client. For that matter, opening lots of tabs really quickly will only trigger one update in Chrome. |
I'm not against delaying the update a bit for performance reasons (or at least suggest it can be low priority), but I don't think it fixes the original issue. Imagine:
Even with the delay, the listener misses the new SW. The only safe way know when a SW becomes installed is to listen for "updatefound" but also track any current |
I think that case is a race between two different operations. I'm not sure devs would expect to duplicate those conditions often. The update on navigate, though, is a very repeatable use case. FWIW, in gecko we have implemented a 1 second delay after document load event. For non-navigation events that trigger an update we fire them 1 second after the event. One an update check is scheduled, any further update checks are coalesced into that currently scheduled timer. We don't push back the timer, though, so that we are guaranteed to fire the update regardless of how many events the SW is receiving. I think this is close to what chrome is doing, but not 100% sure. |
Chrome's implementation tries to schedule the updates after the worker stops:
Scheduling an update pushes back the timer. I implemented this before the spec required Soft Updates at the end of each event. I wanted to wait for the worker to stop to avoid situations where the worker triggers an update and skipWaiting() kills it while it's in middle of an event (Chrome doesn't wait for the active worker to be finish in-progress requests... that's a bug). But as it's approximately the same as the spec (the rough result is a Soft Update approx every 24 hours), I probably wouldn't expend much effort to get it to exactly match unless there's a real benefit for devs. |
F2F resolution: Add a note to the spec to suggest delaying triggering an update until after "DOMContentLoaded" of the associated page. If it's a worker, don't update until the after the worker script has run. |
Recently we got a bug report for the following demo:
https://adriftwith.me/sandbox/service_worker_reload/go.html
When you refresh in chrome you always get the "new ServiceWorker installed" text. Usually it seems after some delay.
In firefox, however, you usually don't get the text, but sometimes you do.
I believe this is happening because we trigger the update immediately after the fetch event resolves. This means the update can complete before the document script has a chance to attach to the registration objects. So it never sees the updatefound events.
I agree with the developers that it would be nice to reliably get updatefound events if you are updating on navigation.
Should the spec be changed to delay navigation updates or the navigation updatefound event until the target document has completed loading?
The text was updated successfully, but these errors were encountered: