Add cacheName support. #9
Conversation
With this change, cacheName is supported. It means that if there is multiple cache storage and developers want to choose which storage to use, they can specify the storage by cacheName.
|
@sisidovski @domenic. Can I ask you to review this PR? |
docs/index.bs
Outdated
| 1. Else if |source| is {{RouterSourceEnum/"cache"}}, or |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=], then: | ||
| 1. [=map/For each=] |cacheName| → |cache| of the [=relevant name to cache map=]: | ||
| 1. If |source|["{{RouterSourceDict/cacheName}}"] [=map/exists=] and |source|["{{RouterSourceDict/cacheName}}"] does not match |cacheName|, [=continue=]. | ||
| 1. Let |requestResponses| be the result of running [=Query Cache=] with |request|, a new {{CacheQueryOptions}},and |cache|. |
There was a problem hiding this comment.
|cache| is going to be passes as |targetStorage| in Query Cache. Not sure if |cache| is data corresponding to the request response list.
https://pr-preview.s3.amazonaws.com/yoshisatoyanagisawa/ServiceWorker/pull/9.html#dfn-request-response-list
There was a problem hiding this comment.
In https://pr-preview.s3.amazonaws.com/yoshisatoyanagisawa/ServiceWorker/pull/9.html#ref-for-dfn-request-response-list%E2%91%A3, |cache| is a request response list, which is set as a value of relevant name to cache map[cacheName]. From that, I assumed that this |cache| is also request response list.
caches.open() returns a promise with Cache that represents a request response list according to https://pr-preview.s3.amazonaws.com/yoshisatoyanagisawa/ServiceWorker/pull/9.html#cache-storage-open.
It is usable with cache.matchAll() (https://pr-preview.s3.amazonaws.com/yoshisatoyanagisawa/ServiceWorker/pull/9.html#dom-cache-matchall).
The "Query Cache" explanation says that:
If the optional argument targetStorage is omitted, set storage to the relevant request response list.
relevant request response list should be request response list for this according to https://pr-preview.s3.amazonaws.com/yoshisatoyanagisawa/ServiceWorker/pull/9.html#dfn-relevant-request-response-list.
Therefore, if |cache| is passed, it should be the same meaning with open the relevant cache and passing its relevant request response list.
There was a problem hiding this comment.
I had the same question. I think you understand this better than me, but one thing that makes me unsure, is that there is only one other place in the spec where "Query Cache" has its third parameter passed: "Batch Cache Operations". All others use the default.
Are we sure that "Batch Cache Operations" is the right model, instead of, e.g., matchAll?
There was a problem hiding this comment.
I picked the steps from CacheStorage.match() and Cache.match(), and Cache.matchAll(). I did not picked the steps in "Batch Cache Operations".
Since this PR do not instantiate Cache, I need to pick a relevant |targetStorage|.
As you mention, the third parameter of "Query Cache" is rarely used, and I understand your concern.
"Query Cache" is used by three locations:
Except for the Batch Cache Operation, they are used as Cache's method. Then, |targetStorage| is filled at https://w3c.github.io/ServiceWorker/#ref-for-dfn-relevant-request-response-list%E2%91%A1. As you can see there, it is this(=Cache)'s request response list.
My PR just did it without using this argument and promise.
There was a problem hiding this comment.
Thank you very much for the clear explanation! LGTM then.
There was a problem hiding this comment.
Thanks for the review. Can I ask you to stamp approved?
With this change, cacheName is supported. It means that if there is multiple cache storage and developers want to choose which storage to use, they can specify the storage by cacheName.
This change also contains some fixes of the behavior. In the previous PR, I misunderstood the cache match
behavior, and it actually might not match multiple cache storage's data.
💥 Error: 403 Forbidden 💥
PR Preview failed to build. (Last tried on Feb 1, 2024, 6:12 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 CSS Spec Preprocessor - CSS Spec Preprocessor is the web service used to build Bikeshed specs.
🔗 Related URL
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.