Re: [w3c/ServiceWorker] Ensure the addRoutes() to be called within the install event (PR #1744)

@domenic commented on this pull request.



> @@ -1605,15 +1605,27 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
 
       The <dfn method for="InstallEvent"><code>addRoutes(|rules|)</code></dfn> method steps are:
 
-        1. Let |serviceWorker| be the [=current global object=]'s associated [=ServiceWorkerGlobalScope/service worker=].
-        1. Let |routerRules| be a copy of |serviceWorker|'s [=list of router rules=].
+        1. Let |event| be [=this=].
+        1. If |event|'s [=dispatch flag=] is unset, [=throw=] an "{{InvalidStateError}}" {{DOMException}}.
+        1. If |rules| is not either of a {{RouterRule}} dictionary or a sequence of {{RouterRule}} dictionaries, then return [=a promise rejected with=] a {{TypeError}}.

This is unnecessary as the IDL layer ensures it will always be true.

>          1.  For each |rule| of |rules|:
             1. If running the [=Verify Router Condition=] algorithm with |rule|["{{RouterRule/condition}}"] and |serviceWorker| returns false, return [=a promise rejected with=] a {{TypeError}}.
-            1. Append |rule| to |routerRules|.
-        1. If |routerRules| [=list/contains=] a {{RouterRule}} whose {{RouterRule/source}} is "{{RouterSourceEnum/fetch-event}}" and |serviceWorker|'s [=set of event types to handle=] does not [=set/contain=] {{ServiceWorkerGlobalScope/fetch!!event}}, return [=a promise rejected with=] a {{TypeError}}.
-        1. Set |serviceWorker|'s [=service worker/list of router rules=] to |routerRules|.
-        1. Return [=a promise resolved with=] undefined.
+        1. If |rules| [=list/contains=] a {{RouterRule}} whose {{RouterRule/source}} is "{{RouterSourceEnum/fetch-event}}" and |serviceWorker|'s [=set of event types to handle=] does not [=set/contain=] {{ServiceWorkerGlobalScope/fetch!!event}}, return [=a promise rejected with=] a {{TypeError}}.

Optional: you could merge this into the previous loop.

>          1.  For each |rule| of |rules|:
             1. If running the [=Verify Router Condition=] algorithm with |rule|["{{RouterRule/condition}}"] and |serviceWorker| returns false, return [=a promise rejected with=] a {{TypeError}}.
-            1. Append |rule| to |routerRules|.
-        1. If |routerRules| [=list/contains=] a {{RouterRule}} whose {{RouterRule/source}} is "{{RouterSourceEnum/fetch-event}}" and |serviceWorker|'s [=set of event types to handle=] does not [=set/contain=] {{ServiceWorkerGlobalScope/fetch!!event}}, return [=a promise rejected with=] a {{TypeError}}.
-        1. Set |serviceWorker|'s [=service worker/list of router rules=] to |routerRules|.
-        1. Return [=a promise resolved with=] undefined.
+        1. If |rules| [=list/contains=] a {{RouterRule}} whose {{RouterRule/source}} is "{{RouterSourceEnum/fetch-event}}" and |serviceWorker|'s [=set of event types to handle=] does not [=set/contain=] {{ServiceWorkerGlobalScope/fetch!!event}}, return [=a promise rejected with=] a {{TypeError}}.
+        1. Let |promise| be a new [=promise=].
+        1. [=ExtendableEvent/Add lifetime promise=] |promise| to |event|.
+
+            Note: {{InstallEvent/addRoutes(rules)|event.addRoutes(rules)}} extends the lifetime of the event by default as if {{ExtendableEvent/waitUntil()|event.waitUntil(promise)}} is called.
+
+        1. Run the following substeps [=in parallel=]:

This feels like it might be a little racy. Perhaps you should use the [parallel queue](https://html.spec.whatwg.org/multipage/infrastructure.html#starting-a-new-parallel-queue) infrastructure whenever you modify the list of router rules? Or maybe even when you access it as well?

I am curious how this maps to the implementation.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1744#pullrequestreview-2571716280
You are receiving this because you are subscribed to this thread.

Message ID: <w3c/ServiceWorker/pull/1744/review/2571716280@github.com>