Skip to content
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

ServiceWorker static routing API #1701

Merged
merged 59 commits into from Feb 22, 2024

Conversation

yoshisatoyanagisawa
Copy link
Contributor

@yoshisatoyanagisawa yoshisatoyanagisawa commented Jan 15, 2024

This API allows developers to configure the routing, and allows them to offload simple things ServiceWorkers do. If the condition matches, the navigation happens without starting ServiceWorkers or executing JavaScript, which allows web pages to avoid performance penalties due to ServiceWorker interceptions.

Explainer: https://github.com/WICG/service-worker-static-routing-api
TAG review: w3ctag/design-reviews#863


💥 Error: 503 Service Unavailable 💥

PR Preview failed to build. (Last tried on Feb 22, 2024, 12:13 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 HTML Diff Service - The HTML Diff Service is used to create HTML diffs of the spec changes suggested in a pull request.

🔗 [Related URL]([object Object])

<html><body><h1>503 Service Unavailable</h1>
No server is available to handle this request.
</body></html>

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

I used to work on the specification update to support the
ServiceWorker static routing API
(https://github.com/WICG/service-worker-static-routing-api)
w3c#1686

However, I accidentally closed it by force-sync to the ServiceWorker
specification's repository HEAD.

This CL is for reviving it.
Just trying to add workflows rule to run CI in static_routing_api repository.
Considering the future update of the condition, we should mark the case
handled as one of conditions.  Also, we need to make it extensible.
It was flipped by mistake.
Copy link
Collaborator

@mkruisselbrink mkruisselbrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a couple of hopefully minor points/questions, otherwise this looks good to me.

docs/index.bs Outdated
@@ -1077,7 +1080,9 @@ spec: storage; urlPrefix: https://storage.spec.whatwg.org/
};
</pre>

A {{ServiceWorkerGlobalScope}} object represents the global execution context of a [=/service worker=]. A {{ServiceWorkerGlobalScope}} object has an associated <dfn export for="ServiceWorkerGlobalScope">service worker</dfn> (a [=/service worker=]). A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">force bypass cache for import scripts flag</dfn>. It is initially unset.
A {{ServiceWorkerGlobalScope}} object represents the global execution context of a [=/service worker=]. A {{ServiceWorkerGlobalScope}} object has an associated <dfn export for="ServiceWorkerGlobalScope">service worker</dfn> (a [=/service worker=]). A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">force bypass cache for import scripts flag</dfn>. A {{ServiceWorkerGlobalScope}} object has an associated <dfn for="ServiceWorkerGlobalScope">race response map</dfn> which is an [=ordered map=] where the [=map/keys=] are [=/requests=] and the [=map/values=] are [=race response=]. It is initially unset.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you re-format this so every "A {{ServiceWorkerGlobalScope}} object has" sentence starts on a new line? And maybe even have blank lines between them so they also start on new lines/paragraphs in the generated HTML. I think that might be more readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also moved It is initially unset. to the correct field.

docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated
1. Else if |source| is {{RouterSourceEnum/"race-network-and-fetch-handler"}}, and |request|'s [=request/method=] is \`<code>GET</code>\` then:
1. Let |queue| be an empty [=queue=] of [=/response=].
1. Let |raceFetchController| be null.
1. Set |raceResponse|'s [=race response/value=] to "<code>pending</code>".
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did |raceResponse| come from? Should this be Let |raceResponse| be a [=race response=] whose [=race response/value=] is "<code>pending</code>".?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

|raceResponse| is declared in L3156.

Let |raceResponse| be a [=race response=] whose [=race response/value=] is null.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, but does it need to be defined at that level? It's only ever non-null in this "race-network-and-fetch-handler" branch of the algorithm, so it could be entirely local to this branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my understanding we can't. |raceResponse| is used in L3247, which is outside of this level.

  1. Return the result of [=Create Fetch Event and Dispatch=] with |request|, |registration|, |useHighResPerformanceTimers|, |timingInfo|, |workerRealm|, |reservedClient|, |preloadResponse|, and |raceResponse|.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But while it is used there, afaik it is always null? In which case that line might be clearer just directly passing null anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

1. Let |queue| be an empty [=queue=] of [=/response=].
1. Let |raceFetchController| be null.
1. Set |raceResponse|'s [=race response/value=] to "<code>pending</code>".
1. Run the following substeps [=in parallel=], but [=abort when=] |controller|'s [=fetch controller/state=] is "<code>terminated</code>" or "<code>aborted</code>":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(mostly an FYI, if #1705 lands first, |controller| will need to be updated to |fetchController|. If this PR lands first, I'll need to update the other PR to also change this here).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. I'll update it once the other PR lands firsts.

docs/index.bs Show resolved Hide resolved
docs/index.bs Outdated
1. Let |shouldSoftUpdate| be true if any of the following are true, and false otherwise:
* |request| is a [=non-subresource request=].
* |request| is a [=subresource request=] and |registration| is [=stale=].
1. Let |raceResponseMap| be |activeWorker|'s [=service worker/global object=]'s [=race response map=].
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should move till after the run Service Worker step below as well to make sure global object has been initialized.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed |raceResponseMap| at this point, and directly call |activeWorker|'s [=service worker/global object=]'s [=race response map=] after the global object has been initialized.

sisidovski and others added 4 commits February 21, 2024 10:45
Co-authored-by: Marijn Kruisselbrink <mek@chromium.org>
… unset` phrase (#14)

* Remove |raceResponseMap|

* Re-format {{ServiceWorkerGlobalScope}} part and move `It is initially unset` phrase
Upon the review request, this CL makes |activeWorker| not modified
in the Setup ServiceWorkerGlobalScope algorithm.  It is the caller's
responsibility to update the active service worker's global scope.

With this change, an effemeral ServiceWorkerGlobalScope can be used
for a cache source in the static routing API.
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated
:: |workerRealm|, a [=relevant realm=] of the [=service worker/global object=]
:: |reservedClient|, a [=request/reserved client=]
:: |preloadResponse|, a [=promise=]
:: |raceResponse|, a [=race response=]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add "or null", since sometimes you pass in null (and handle null)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

docs/index.bs Outdated
: Output
:: a [=/response=] or null

1. If |request|'s [=request/reserved client=] is null, return null.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe add a Note explaining that this check ensures that we're only dealing with non-subresrouce requests, and thus looking up a registration using the request URL is correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was wrong. We don't want to limit this algorithm for non-subresource requests. What we'd like to achieve here is to get [=active worker=] from [=/request=], regardless of non-subresource requests or subresource requests.

Updated to get the active worker from subreosurce requests, but perhaps we can just use |request|'s [=request/client=]'s [=active service worker=] for both subresource and non-subresource requests?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you have now looks reasonable to me. I think using request's client's active service worker for non-subresource requests would be incorrect. Maybe at some point we should abstract out a "give me the right registration for this request" algorithm to be shared between Handle Fetch and this algorithm, but for now what you have seems fine too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification!

* Remove |raceResponseMap|

* Re-format {{ServiceWorkerGlobalScope}} part and move `It is initially unset` phrase

* Fix |url| in Lookup Race Response

* Update race handling, lookup race response algorithm
Copy link
Collaborator

@mkruisselbrink mkruisselbrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience, I think this addressed all my concerns.

@mkruisselbrink mkruisselbrink merged commit 8d4b9df into w3c:main Feb 22, 2024
2 checks passed
github-actions bot added a commit that referenced this pull request Feb 22, 2024
SHA: 8d4b9df
Reason: push, by mkruisselbrink

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yoshisatoyanagisawa yoshisatoyanagisawa deleted the static_routing_api branch February 22, 2024 08:40
tidoust added a commit to tidoust/ServiceWorker that referenced this pull request Feb 22, 2024
Following w3c#1701, the `install` event now uses the `InstallEvent` interface.
github-actions bot added a commit to asleekgeek/ServiceWorker that referenced this pull request Feb 22, 2024
SHA: 8d4b9df
Reason: push, by pull[bot]

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
mkruisselbrink pushed a commit that referenced this pull request Feb 22, 2024
Following #1701, the `install` event now uses the `InstallEvent` interface.
sisidovski pushed a commit to whatwg/urlpattern that referenced this pull request Feb 29, 2024
In w3c/ServiceWorker#1701, the ServiceWorker static routing API uses the match algorithm to match the request URL. However, the original match algorithm accepts a URLPatternInput object, and it does not accept a URL struct. To allow the ServiceWorker static routing API to use the match algorithm, we need to make the match algorithm to accept the URL struct.

Fixes: #218.
yoshisatoyanagisawa added a commit to yoshisatoyanagisawa/ServiceWorker that referenced this pull request Mar 4, 2024
Updates in the URLPattern specification was requested during the
w3c#1701 review.

The issues for the request has been resolved:
- whatwg/urlpattern#217
- whatwg/urlpattern#218

However, during the specification updates, the algorithm names are also
updated, and adjustment is needed.
mkruisselbrink pushed a commit that referenced this pull request Mar 13, 2024
…tion (#1707)

* Update link texts upon updates in the URLPattern specification

Updates in the URLPattern specification was requested during the
#1701 review.

The issues for the request has been resolved:
- whatwg/urlpattern#217
- whatwg/urlpattern#218

However, during the specification updates, the algorithm names are also
updated, and adjustment is needed.


Co-authored-by: Domenic Denicola <d@domenic.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants