-
Notifications
You must be signed in to change notification settings - Fork 319
ServiceWorkerAutoPreload #1756
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
ServiceWorkerAutoPreload #1756
Conversation
@yoshisatoyanagisawa @youennf I think now it's ready for the review. Could you take a look? Thank you! |
1. [=If aborted=], then: | ||
1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|. | ||
1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|. | ||
1. If |request| is a [=navigation request=], |request|'s [=request/method=] is \`<code>GET</code>\`, |registration|'s [=active worker=]'s [=set of event types to handle=] [=set/contains=] <code>fetch</code>, and |registration|'s [=active worker=]'s [=all fetch listeners are empty flag=] is not set 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.
Ah, you explicitly skip subresource auto preload. just for note to myself.
https://github.com/WICG/service-worker-auto-preload?tab=readme-ov-file#auto-preload-for-subresources
No need to do that in this PR, but I feel steps to create |raceResponse| can be merged with "race-network-and-fetch-event" case. |
1. [=If aborted=], then: | ||
1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|. | ||
1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|. | ||
1. Else if |timingInfo|'s [=service worker timing info/worker matched router source=] is not "{{RouterSourceEnum/fetch-event}}", a user agent may run the following substeps: |
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.
Updated not to introduce |source| here, use |timingInfo|'s [=service worker timing info/worker matched router source=] instead.
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.
Thanks.
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.
Why doing autopreload if the route is NOT fetch-event
?
I would expect to only do autopreload if the route is actually fetch-event
.
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.
We frame the autopreload is an optional optimization that the browser can apply at its choosing. So we want to use the static routing API as an opt-out mechanism. If we only invoke the autopreload only when the matched router source is fetch-event
, that is more like an opt-in, not opt-out.
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.
Ah I see, I was mislead thinking that, at this point, the source could be other values like cache
and so on, but it can only be null
or fetch-event
really.
Maybe, it would be clearer if we would check if worker matched router source
is null
?
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.
Thank you! That sounds reasonable. Added Assert: [=service worker timing info/worker matched router source=] is null.
.
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.
lgtm.
The specification change itself looks good to me. However, let me wait for other vendors to agree the change. |
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.
Looks ok overall to me, some thoughts inline
1. [=If aborted=], then: | ||
1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|. | ||
1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|. | ||
1. Else if |timingInfo|'s [=service worker timing info/worker matched router source=] is not "{{RouterSourceEnum/fetch-event}}", a user agent may run the following substeps: |
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.
Why doing autopreload if the route is NOT fetch-event
?
I would expect to only do autopreload if the route is actually fetch-event
.
1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|. | ||
1. Else if |timingInfo|'s [=service worker timing info/worker matched router source=] is not "{{RouterSourceEnum/fetch-event}}", a user agent may run the following substeps: | ||
|
||
Note: A user agent may speculatively dispatch a network request in parallel with creating a fetch event in order to minimize the bootstrap cost. |
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.
Is there sufficient description on when to do this speculative load?
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.
In the explainer, we mention two possible scenarios that the browser may invoke the autopreload.
https://github.com/WICG/service-worker-auto-preload?tab=readme-ov-file#eligibility-criteria
I don't think we can fully specify when it's used since some conditions are not deterministic, but do you think we should say more here?
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.
That might be ok as is. If there are cases where all browsers plan to do autopreload, we could be more precise, maybe with a SHOULD for instance. Say if the service worker is not running?
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.
Yes. Let's keep it as is for now.
Just to note: we're happy with rephrasing it with SHOULD.
docs/index.bs
Outdated
1. Set |responseForAutoPreload| to be a [=race response=] whose [=race response/value=] is "<code>pending</code>". | ||
1. Run the following substeps [=in parallel=], but [=abort when=] |fetchController|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>": | ||
1. Set |autoPreloadFetchController| to the result of calling [=fetch=] given |request|, with [=fetch/processResponse=] set to the following steps given a [=/response=] |autoPreloadRequestResponse|: | ||
1. If |autoPreloadRequestResponse|'s [=response/status=] is [=ok status=], set |responseForAutoPreload|'s [=race response/value=] to |autoPreloadRequestResponse|. |
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.
Why only using the auto preload response if it is a 2XX response?
What happens if the auto preload response is not a 2XX (I haven't yet looked precisely at how the race response mechanism works)?
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.
Ah, good catch!
I guess |autoPreloadRequestResponse| should be set to |responseForAutoPreload|'s value regardless of the response status. Otherwise, deadlock at Step 21-3 in https://w3c.github.io/ServiceWorker/#create-fetch-event-and-dispatch-algorithm
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.
Thanks! Updated to set the response to responseForAutoPreload
regardless of the status.
@youennf Could you take a look this again? Thanks! |
LGTM overall. Two suggestions as detailed in the comments, but they are not blockers. |
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.
Thanks!
1. [=If aborted=], then: | ||
1. Let |deserializedError| be the result of [=deserialize a serialized abort reason=] given null and |workerRealm|. | ||
1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|. | ||
1. Else if |timingInfo|'s [=service worker timing info/worker matched router source=] is not "{{RouterSourceEnum/fetch-event}}", a user agent may run the following substeps: |
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.
Thank you! That sounds reasonable. Added Assert: [=service worker timing info/worker matched router source=] is null.
.
1. [=fetch controller/Abort=] |preloadFetchController| with |deserializedError|. | ||
1. Else if |timingInfo|'s [=service worker timing info/worker matched router source=] is not "{{RouterSourceEnum/fetch-event}}", a user agent may run the following substeps: | ||
|
||
Note: A user agent may speculatively dispatch a network request in parallel with creating a fetch event in order to minimize the bootstrap cost. |
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.
Yes. Let's keep it as is for now.
Just to note: we're happy with rephrasing it with SHOULD.
ok, I think all comments are addressed. |
SHA: 991cb02 Reason: push, by yoshisatoyanagisawa Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
SHA: 991cb02 Reason: push, by pull[bot] Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This is the PR to support ServiceWorkerAutoPreload.
ServiceWorkerAutoPreload is defined as an optional browser optimization so that the browser minimizes the cost of the bootstrap by dispatching the navigational network request before starting the service worker. The dispatched network request is consumed by
fetch(e.request)
in the fetch handler after the bootstrap, or consumed by the client as a fallback request if the fetch handler doesn't callrespondWith()
.The developer can disable this optimization by opting out via setting the router rule provided by the static routing API. Specifically, the router source "network" works as an opt-out signal.
Please read the explainer for more detail.
Preview | Diff