-
Notifications
You must be signed in to change notification settings - Fork 320
Opt cache.add/addAll and importScripts out of a local service worker #1025
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
Conversation
|
Once this lands I still need to expose service worker clients to fetch events. |
jungkees
left a comment
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.
So, this assumes https://fetch.spec.whatwg.org/#concept-http-fetch will change to invoke Handle Fetch even if the client is a SW, right? For that, I think this patch can still land as setting the skip-service-worker flag makes it bypass the Handle Fetch. But I would like to confirm we expect add()/addAll() and importScripts() go through foreign fetch, right?
jungkees
left a comment
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.
Would like to check if you wanted to introduce a new flag.
docs/index.bs
Outdated
| 1. If |r|'s [=request/url=]'s [=url/scheme=] is not one of "<code>http</code>" and "<code>https</code>", then: | ||
| 1. [=fetch/Terminate=] all the ongoing <a>fetches</a> initiated by |requests| with reason *fatal*. | ||
| 1. Break the loop. | ||
| 1. If |r|'s [=request/client=]'s [=environment settings object/global object=] is a {{ServiceWorkerGlobalScope}} object, set |request|'s [=skip-local-service-worker flag=]. |
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.
Are you introducing a new flag here? Or did you intend to use skip-service-worker flag?
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.
See whatwg/fetch#435
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.
OK. Missed that. I'll check that.
docs/index.bs
Outdated
| 1. Let |serviceWorker| be |request|'s [=request/client=]'s [=environment settings object/global object=]'s [=ServiceWorkerGlobalScope/service worker=]. | ||
| 1. If |serviceWorker|'s <a>imported scripts updated flag</a> is unset, then: | ||
| 1. Let |registration| be |serviceWorker|'s [=containing service worker registration=]. | ||
| 1. Set |request|'s [=skip-local-service-worker flag=]. |
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.
Same as above.
|
Also I think V1 should also take this behavior. Could you apply it to V1? |
|
Note whatwg/fetch#435 (comment) - skip-local-service-worker is going to change to become an enum. |
|
Yeah I'll add it to v1 too |
87b93fc to
ccd10f2
Compare
This allows certain fetches within service workers to trigger fetch events. It also makes interception of redirects by foreign fetch possible. Tests: web-platform-tests/wpt#4518. Related service worker PR: w3c/ServiceWorker#1025. Fixes #303 and fixes #362.
|
@jungkees The tests & fetch spec part of this have merged now. Happy for this to merge too? |
ccd10f2 to
2532cb6
Compare
Fixes whatwg/fetch#303 (comment).
Related fetch PR whatwg/fetch#435.