Re: [w3c/ServiceWorker] Improve service worker script caching and update (#1283)

mattto commented on this pull request.



> +        1. Set |request|'s [=request/cache mode=] to "<code>no-cache</code>" if any of the following are true:
+            * |registration|'s [=service worker registration/update via cache mode=] is "`none`".
+            * The [=current global object=]'s [=force bypass cache for importscripts flag=] is set.
+            * |registration|'s [=last update check time=] is not null and the time difference in seconds calculated by the current time minus |registration|’s [=last update check time=] is greater than 86400.
+        1. Let |response| be the result of <a lt="fetch">fetching</a> |request|.
+        1. If |response|’s <a for="response" href="https://github.com/whatwg/fetch/issues/376">cache state</a> is not "<code>local</code>", set |registration|’s [=service worker registration/last update check time=] to the current time.
+        1. [=Extract a MIME type=] from the |response|'s [=unsafe response=]'s [=response/header list=]. If this MIME type (ignoring parameters) is not a [=JavaScript MIME type=], return a [=network error=].
+        1. If |response|'s <a>unsafe response</a>'s [=response/type=] is not "<code>error</code>", and |response|'s [=response/status=] is an <a>ok status</a>, then:
+            1. Let |newestWorker| be the result of running [=Get Newest Worker=] with |registration|.
+            1. Let |resource| be null.
+            1. If |newestWorker| is not null, set |resource| to |newestWorker|'s [=service worker/script resource map=][|request|'s [=request/url=]], or null if it does not [=map/exist=].
+            1. Set |serviceWorker|'s [=service worker/include updated resources flag=] if any of the following are true:
+                * |newestWorker| is null.
+                * |resource| is null.
+                * |resource|'s [=response/body=] is not byte-for-byte identical with |response|'s [=response/body=]. 
+            1. [=map/Set=] [=service worker/script resource map=][|request|'s [=request/url=]] to |response|.

This should be "Set *serviceWorker's* script resource map", right?

> -        1. Else:
-            1. If <a>script resource map</a>[|url|] [=map/exists=], return <a>script resource map</a>[|url|].
-            1. Else, return a <a>network error</a>.
+        1. If |serviceWorker|'s [=service worker/script resource map=][|request|'s [=request/url=]] [=map/exists=], return [=service worker/script resource map=][|request|'s [=request/url=]].
+        1. If |serviceWorker|'s [=state=] is *installed*, *activating*, *activated*, or *redundant*, return a [=network error=].
+        1. Let |registration| be |serviceWorker|'s [=containing service worker registration=].
+        1. Set |request|'s [=service-workers mode=] to "`none`".
+        1. Set |request|'s [=request/cache mode=] to "<code>no-cache</code>" if any of the following are true:
+            * |registration|'s [=service worker registration/update via cache mode=] is "`none`".
+            * The [=current global object=]'s [=force bypass cache for importscripts flag=] is set.
+            * |registration|'s [=last update check time=] is not null and the time difference in seconds calculated by the current time minus |registration|’s [=last update check time=] is greater than 86400.
+        1. Let |response| be the result of <a lt="fetch">fetching</a> |request|.
+        1. If |response|’s <a for="response" href="https://github.com/whatwg/fetch/issues/376">cache state</a> is not "<code>local</code>", set |registration|’s [=service worker registration/last update check time=] to the current time.
+        1. [=Extract a MIME type=] from the |response|'s [=unsafe response=]'s [=response/header list=]. If this MIME type (ignoring parameters) is not a [=JavaScript MIME type=], return a [=network error=].
+        1. If |response|'s <a>unsafe response</a>'s [=response/type=] is not "<code>error</code>", and |response|'s [=response/status=] is an <a>ok status</a>, then:
+            1. Let |newestWorker| be the result of running [=Get Newest Worker=] with |registration|.

Wouldn't newestWorker just be serviceWorker, it's the new installing worker?

> -            1. Else, return a <a>network error</a>.
+        1. If |serviceWorker|'s [=service worker/script resource map=][|request|'s [=request/url=]] [=map/exists=], return [=service worker/script resource map=][|request|'s [=request/url=]].
+        1. If |serviceWorker|'s [=state=] is *installed*, *activating*, *activated*, or *redundant*, return a [=network error=].
+        1. Let |registration| be |serviceWorker|'s [=containing service worker registration=].
+        1. Set |request|'s [=service-workers mode=] to "`none`".
+        1. Set |request|'s [=request/cache mode=] to "<code>no-cache</code>" if any of the following are true:
+            * |registration|'s [=service worker registration/update via cache mode=] is "`none`".
+            * The [=current global object=]'s [=force bypass cache for importscripts flag=] is set.
+            * |registration|'s [=last update check time=] is not null and the time difference in seconds calculated by the current time minus |registration|’s [=last update check time=] is greater than 86400.
+        1. Let |response| be the result of <a lt="fetch">fetching</a> |request|.
+        1. If |response|’s <a for="response" href="https://github.com/whatwg/fetch/issues/376">cache state</a> is not "<code>local</code>", set |registration|’s [=service worker registration/last update check time=] to the current time.
+        1. [=Extract a MIME type=] from the |response|'s [=unsafe response=]'s [=response/header list=]. If this MIME type (ignoring parameters) is not a [=JavaScript MIME type=], return a [=network error=].
+        1. If |response|'s <a>unsafe response</a>'s [=response/type=] is not "<code>error</code>", and |response|'s [=response/status=] is an <a>ok status</a>, then:
+            1. Let |newestWorker| be the result of running [=Get Newest Worker=] with |registration|.
+            1. Let |resource| be null.
+            1. If |newestWorker| is not null, set |resource| to |newestWorker|'s [=service worker/script resource map=][|request|'s [=request/url=]], or null if it does not [=map/exist=].

I'm understanding more.

I thought when we discussed this a while back (https://github.com/w3c/ServiceWorker/issues/839#issuecomment-266373359), we decided the "update" service worker shouldn't run at all if the main script and imported scripts of the newest installed worker haven't changed. I.e., the update check should first fetch the main script and importScripts of the newest worker, if those haven't changed you halt the update. If there is a change you launch the new service worker. It'd be wasteful to spawn a worker just to do the importScripts then discover there was no change, and then terminate the worker before installation. But maybe we should talk about this one of the GitHub issues.

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