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

Wait for an active worker on .register (consistency with push API) #101

Closed
jakearchibald opened this issue Oct 12, 2015 · 5 comments
Closed

Comments

@jakearchibald
Copy link
Collaborator

Between steps 2 & 3 of https://slightlyoff.github.io/BackgroundSync/spec/#in-the-background, perform step 5 of https://w3c.github.io/push-api/#widl-PushManager-subscribe-Promise-PushSubscription--PushSubscriptionOptions-options

This prevents .register appearing to succeed, but never getting an active worker to fire a sync event in.

@jkarlin
Copy link
Collaborator

jkarlin commented Oct 12, 2015

I believe you mean between steps 2 & 3 of https://slightlyoff.github.io/BackgroundSync/spec/#dom-syncmanager-register perform step 5 of Push's subscribe method (the link is totally broken for some reason).

Hmm, should we instead just fail if the registration isn't active?

@jakearchibald
Copy link
Collaborator Author

Hmm, sorry for the broken links. The first is my fault, not sure what's going on with the second.

Hmm, should we instead just fail if the registration isn't active?

That's what I thought push did, but seems they wait. @beverloo I guess there was some discussion around this?

@beverloo
Copy link
Member

Yes. One flow that provided a lot of early feedback was the following:

navigator.serviceWorker.register(..).then(registration => {
  registration.pushManager.subscribe(..);
});

Which, on first load, would fail because the registration hadn't been activated yet. Instead, the developer had to do the following:

navigator.serviceWorker.register(...);
navigator.serviceWorker.ready.then(registration => {
  registration.pushManager.subscribe(..);
});

Many of the Service Workers we saw for Push didn't have oninstall/onactivate/onfetch events at all. This probably is less of a concern for background sync (which'd depend on the caches?), but still something to consider.

@jkarlin
Copy link
Collaborator

jkarlin commented Oct 12, 2015

I can imagine many cases where the cache isn't needed (e.g., for puts instead of gets) for background sync. Waiting for the active registration works for me if that's the norm.

@mkruisselbrink
Copy link
Collaborator

(The second link was broken because it's a respec spec, so the link target just doesn't exist until some time after the page has loaded...)

I'm not sure what the benefit would be of failing if the registration doesn't have an active worker yet?
There are only very few situations where this could possibly trigger: 1) a new SW registration, which has finished its oninstall event, but hasn't started its onactivate event yet. Or 2) a new SW trying to register sync from its own oninstall event.
In the first case I don't think there isn't really any possible delay. The new worker will become the active worker (which only means it has started activating) almost immediately, so rejecting sync registration doesn't seem helpful.
In the second case of course it is a bit more complicated, since installation can still fail at that point, but that case also seems unlikely enough that I don't think it really matters what behavior that ends up having.

So I agree that yes, register should wait for the registration to have an active worker.

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

No branches or pull requests

4 participants