-
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
Fetch service worker scripts with "no-cache" by default #1020
Conversation
This introduces service worker registration's use cache field and its related APIs: options.useCache to register method and registration.useCache for ServiceWorkerRegistration objects. This changes the default cache mode of fetching SW scripts to "no-cache". After the change, 24 hours limit and job's force bypass cache flag rules are still enforced. register() method's options value set to { useCache: true } re-enables the previous default behavior provided before this change. Fixes #893.
/cc @wanderview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good, but it needs to request imported scripts no-cache too.
@@ -187,8 +187,6 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe | |||
|
|||
A [=/service worker registration=] has an associated <dfn export id="dfn-scope-url">scope url</dfn> (a [=/URL=]). | |||
|
|||
A [=/service worker registration=] has an associated <dfn export id="dfn-registration-script-url">registering script url</dfn> (a [=/URL=]). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, good catch!
@@ -1636,7 +1644,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe | |||
1. If the "<code>scope</code>" <a>target attribute</a> of the <code>Link</code> header is present, set |scopeURL| to the result of <a lt="URL parser">parsing</a> the "<code>scope</code>" <a>target attribute</a> with |scriptURL|. | |||
1. Let |workerType| be the "<code>workertype</code>" <a>target attribute</a> of the <code>Link</code> header, or "<code>classic</code>" if no such attribute is present. | |||
1. If |workerType| is not a valid {{WorkerType}} value, abort these steps. | |||
1. Invoke [=Start Register=] with |scopeURL|, |scriptURL|, a new <a>promise</a>, null, |contextURL| and |workerType|. | |||
1. Let |useCache| be the "<code>usecache</code>" <a>target attribute</a> of the <code>Link</code> header, or false if no such attribute is present. | |||
1. If |useCache| is not a valid boolean value, abort these steps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be an invalid value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any other value except true or false. I copied this from other target attribute's value sanitization steps above. @mkruisselbrink, could you check if this step is needed and done in a right way?
Addressed. Note that job's force bypass cache flag is effective for the script executions in both Update and Install for now. Depending on #1021, there's a chance that the script execution in Install won't need this bypass cache flag. |
I don't think updates are doing the right thing. "Soft update" is never called with force bypass cache flag. "Update" 9.6 will only set force bypass cache for importscripts flag if force bypass cache flag is set, so it ignores use cache on the registration. |
Same goes for "Install" - it doesn't pay attention to use cache on the registration when it comes to |
@@ -2500,7 +2527,7 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe | |||
|
|||
Else, continue the rest of these steps after the algorithm's asynchronous completion, with |script| being the asynchronous completion value. | |||
|
|||
1. If |newestWorker| is not null, |newestWorker|'s [=service worker/script url=] [=url/equals=] |job|'s [=job/script url=] with the *exclude fragments flag* set, and |script| is a byte-for-byte match with |newestWorker|'s <a>script resource</a>, then: | |||
1. If |newestWorker| is not null, |newestWorker|'s [=service worker/script url=] [=url/equals=] |job|'s [=job/script url=] with the *exclude fragments flag* set, and |script|'s [=source text=] is a byte-for-byte match with |newestWorker|'s [=script resource=]'s [=source text=], if |script| is a [=classic script=], and |script|'s [=module record=]'s \[[ECMAScriptCode]] is a byte-for-byte match with |newestWorker|'s [=script resource=]'s [=module record=]'s \[[ECMAScriptCode]] otherwise, then: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bikeshed doesn't seem to pick up "module record"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed: whatwg/html#2157. And put module script specifier for module records: d2047e1.
@jakearchibald, force bypass cache flag is for dev tools, debugging purposes, potential calls from other specs, etc. (See #753.) The registration's use cache field is being checked in Update and |
Ah, I think I was looking at a commit before |
This introduces service worker registration's use cache field and its
related APIs: options.useCache to register method and
registration.useCache for ServiceWorkerRegistration objects. This
changes the default cache mode of fetching SW scripts to "no-cache".
After the change, 24 hours limit and job's force bypass cache flag rules
are still enforced. register() method's options value set to
{ useCache: true } re-enables the previous default behavior provided
before this change.
Fixes #893 #894.