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

Add navigator.serviceWorker.whenReady() #223

Closed
slightlyoff opened this issue Apr 8, 2014 · 26 comments
Closed

Add navigator.serviceWorker.whenReady() #223

slightlyoff opened this issue Apr 8, 2014 · 26 comments

Comments

@slightlyoff
Copy link
Contributor

The semantic is:

  • doesn't fail
  • whenever a SW is installed for the current document (even if it doesn't control the document), it resolves successfully

This allow us to write code that isn't as dependent on navigator.serviceWorker.register() which can register for scopes outside the current document.

This is useful for APIs like background synchronization that will have code like:

navigator.serviceWorker.register("sw.js"); // ...

navigator.serviceWorker.whenReady().then(function(sw) {
  navigator.requestSync("id", {
    description: "",
    data: "",
    interval: 1000 * 60 * 60 * 24,
    once: false,
  }).then(onsuccess, onfailure);
}, neverCalled);
@mfalken
Copy link
Member

mfalken commented May 8, 2014

I'm looking into implementing this. Does the implementation need to guarantee that register() resolves before whenReady()? I.e., from the user's point of view, the function passed to register.then() executes before the function passed to whenReady.then() (in the case where there's not already a registered worker).

I think it may be a general Promises question, can you claim an ordering between two promises that don't have a promise dependency?

@dominiccooney
Copy link

The spec is very operational in that it talks about whenReady resolving when a ServiceWorker "becomes" this or that. It would be good to clarify what happens if the page already has an installed or active ServiceWorker when whenReady is called; presumably this resolves immediately.

Blink will implement this:

If navigator.serviceWorker.waiting is non-null, has state installed, and the statechange event for "installed" is finished, whenReady resolves with that ServiceWorker.

Otherwise, if navigator.serviceWorker.controller is non-null, whenReady resolves with that ServiceWorker.

Otherwise, whenReady resolves when waiting next becomes non-null, has state installed, and the statechange event for "install" is finished.

whenReady may return different promise objects so that the caller gets a "most recent" ServiceWorker, for example in the situation where a page sees multiple controllers through use of replace().

@annevk
Copy link
Member

annevk commented Jun 3, 2014

This should not be a method by the way. As discussed elsewhere with @domenic at all this is simply a state transition. Those can be properties that return a getter.

@slightlyoff
Copy link
Contributor Author

I'm fine with this as a property if @domenic is.

On Tue, Jun 3, 2014 at 12:24 AM, Anne van Kesteren <notifications@github.com

wrote:

This should not be a method by the way. As discussed elsewhere with
@domenic https://github.com/domenic at all this is simply a state
transition. Those can be properties that return a getter.


Reply to this email directly or view it on GitHub
#223 (comment)
.

@domenic
Copy link
Contributor

domenic commented Jun 3, 2014

Indeed, sounds good. It should probably be named ready too.

@dominiccooney
Copy link

@domenic Thanks for the name and context. I will implement something like

interface ServiceWorkerContext ... {
   readonly attribute Promise ready;
   ...

@jungkees
Copy link
Collaborator

jungkees commented Jun 4, 2014

I don't have a strong preference for the naming but would like to recall the point discussed during the f2f (though that day I was not in the room for other meeting):

jungkees: how about whenReady() ==> ready()?
slightlyoff: we talked about it and I think whenReady() is different because ready() implies that it should fail if the SW isn't registered
slightlyoff: the idea with whenReady() is that it won't fail in any case
slightlyoff: but can possibly succeed when a SW becomes active for the current document's scope

So, if .ready carries the meaning well enough itself, it would be fine. Else, we may keep .whenReady. WDYT?

@coonsta the interface name above is ServiceWorkerContainer, right?

@jungkees
Copy link
Collaborator

jungkees commented Jun 4, 2014

Addressed d8d7a8c. Will change name if people prefer to do so.

@dominiccooney
Copy link

@jungkees Right, ServiceWorkerContainer.

Blink has implemented 'ready' but I can rename it.

@annevk
Copy link
Member

annevk commented Jun 4, 2014

ready seems fine to me. It's already on serviceWorker, seems like enough context.

@jungkees
Copy link
Collaborator

jungkees commented Jun 5, 2014

Renamed it to ready and updated the algorithm 641a910

@coonsta from https://github.com/dstockwell/blink/commit/a114f7e9fcbd49f74f8444f872bf61f06c297b28, I would like to clarify ourselves that navigator.serviceWorker.ready resolves with either the active worker object or the waiting worker object rather than the controller. If a document has a controller which is the same as active worker, that's true but in the case it doesn't have a controller, the promise can resolve with an active worker which will be effective upon subsequent navigation. Is that the same as you expected?

@domenic
Copy link
Contributor

domenic commented Jun 6, 2014

IMO these type of promises should not resolve with anything. I do not hold that opinion strongly however, and it is a general one that might not apply well to this case.

@jungkees
Copy link
Collaborator

Understood your point. We still need to mull over whether there's any scenarios for which the resolved value (either an active worker object or a worker in waiting object) would be useful.

As stated in the first comment of the thread, the primary pattern .ready would be used is the registration (initialization) of other APIs (Background synchronization, Push API, Task Scheduler, etc.) relying on a SW which is at least a worker in waiting:

navigator.serviceWorker.ready.then(function(sw) {
  /* bind a feature to SW registration */
});

Currently those proposals do not specify any explicit parameter for SW.

@dominiccooney
Copy link

@jungkees Blink does not support navigator.serviceWorker.active yet, so the implementation of .ready uses .controller instead. When we implement .active I will switch over to use .active instead of .controller.

I am trying to make .ready return the same Promise object per feedback from @annevk on blink-dev. Note that the editor's draft requires a new promise each time. From the implementer's point of view, always returning a new promise would be much simpler. We should have feedback this week about the feasibility of returning a stable promise.

It is easy for Blink to pass the ServiceWorker object when resolving the ready promise or not, right now we do pass it, we will do whatever the spec says here.

@annevk
Copy link
Member

annevk commented Jun 16, 2014

Always returning a new object from an attribute is a non-starter and this really ought to be an attribute. What is the problem with holding on to the promise object? That the C++ class needs to hold onto a JS value? Because we are going to see a ton of that with the way we figured out attributes returning arrays should work.

@dominiccooney
Copy link

@annevk The attempt at returning a stable Promise in Blink is in this code review: https://codereview.chromium.org/337063002/ I think it is basically foundered on the complexity explosion of:

  • 'ready' is halfway between a Promise (one resolution) and a stream (ServiceWorkers can be replaced in a long-lived page)
  • Holding onto the JavaScript Promise object without leaking things
  • Behaving when the frame is navigated
  • All of the above for Chrome's extension mechanism, in addition.

I'll note that Blink does not even get returning a stable Promise right for FontFace.loaded, which is less complicated.

@annevk
Copy link
Member

annevk commented Jun 23, 2014

Paging @domenic @dherman @sicking

The complexity explosion seems like something we ought to solve by getting those aspects defined more clearly. I guess the question is what we do in the meantime.

@domenic
Copy link
Contributor

domenic commented Jun 23, 2014

It's hard to really say what we should do, since the semantics of such "stable promises" are extremely simple from the JS side. That is, a pure-JS implementation would be able to achieve these semantics naturally and easily. This implies to me that the complexity comes from how these JS concepts manifest in a particular browser and C++ mapping, which is engine-specific.

@annevk
Copy link
Member

annevk commented Jun 23, 2014

Paging @bzbarsky @bholley for some opinions from Gecko land.

@bzbarsky
Copy link

@domenic is spot on. I don't think Gecko has any obvious issues here, since we have a memory management mechanism that can handle arbitrary cycles through the combined C++ & JS object graph. Blink instead uses ad-hoc mechanisms for references from C++ to JS, which means any time you add a new kind of such reference it's a nightmare. This is why even FontFace.loaded is rocket science in Blink...

That said, the above only covers extension interaction and leaks. I'd like to better understand what the navigation issues are before saying whether those are a problem or not.

@dominiccooney
Copy link

OK, Blink is (specifically, I am) trying to implement stable promises again. The latter three bullets above might be idiosyncratic implementation issues for Blink.

@dominiccooney
Copy link

FYI I have filed issue #356 which is related to ready.

Separately, if the desire is to have a stable Promise object, the spec needs to state that; today it says "1. Let promise be a newly-created promise."

Since a document can have multiple different active workers over the course of its life, the spec needs to specify how that interacts with the stable ready Promise. I suggest you need internal state associated with the ServiceWorkerContainer for the actual Promise object, and something in the set-active algorithm that says:

If ready is settled:
    Set ready to a newly-created Promise.
Resolve ready with activeWorker.

At this point it looks like Blink can implement whatever you want--stable Promise, stable Promise with reset at whatever time, multiple Promises.

@dominiccooney
Copy link

See also #357 which I filed and relates to ready.

@dominiccooney
Copy link

Implementer feedback: Blink has updated its implementation of ready to return the same Promise as much as possible. We will return a new Promise when:

  • This is the first invocation of ready, or
  • The existing Promise was resolved, and the active Service Worker subsequently changed.

@jakearchibald
Copy link
Contributor

This is done afaik

@KenjiBaheux
Copy link
Collaborator

The new whenReady (i.e. ready) is tracked at http://crbug.com/399533 for Blink

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

No branches or pull requests

9 participants