Closed markbrocato closed 4 years ago
Hello @markbrocato!
Could you (or any of the folks who have đź‘Ť this issue) go into some more detail about the use case if this functionality were added? The implication is that you're using it with some sort of HTTP backend that ignores the bodies of POST
requests (since they wouldn't be saved when used as cache keys) and for which getting back a cached response, instead of a live response from the server, would suffice. I'm not sure how common that sort of POST
usage is, but I'm happy to learn more.
If your use case is primarily motivated by performing some sort of fallback when a POST
request fails, and you don't want to use workbox-background-sync
(which falls back by serializing the request body to IndexedDB and retrying it once the network is available), then perhaps just using a custom plugin implementing fetchDidFail
would be sufficient? It's an async
callback and you could, theoretically, write to the cache inside of that if you really needed to.
We've come across quite a few use cases for caching posts. One that stands out is graphql, which almost always uses post requests. There is no reason my modern CDNs can't use the post body as part of the cache key. We've used Fastly and CloudFront to do this in the past, and I'm sure the other major providers support it. Regardless of that, the client should be able to cache requests that cannot be cached on the server. The cache-control: private flag is a classic example of this.
Other use cases stem from the fact that it's common for apps to disregard http semantics and use posts where gets should be used (I'd argue graphql is just one of many). If you're attempting to add caching to an app that for whatever reason you can't change, the ability to cache posts can be very handy.
Lastly, I feel the particular use cases, while interesting, aren't that important. What the existing code is attempting to do is validate the cache key. Since the workbox api provides the ability to customize the cache key with cacheKeyWillBeUsed, I feel it's more correct to validate the custom key if one is provided, not the default key. The default key may be invalid while the custom key is valid.
Changing the order of the validation may be fine—thanks for the PR to do that. My main issue is framing this as way of allowing developers to cache POST
requests from a service worker, and specifically, GraphQL POST
requests.
While modern CDNs might be able to use the POST
body as part of the cache key, the Cache Storage API that browsers use cannot. This is the algorithm that browsers use when performing a cache match. It's technically possible to use a POST
request as the key when performing a match, but only if ignoreMethod: true
is set. And if you do that, the POST
's body doesn't come into play at all when performing a match. For similar reasons, the Cache Storage API won't let you use a non-GET
request as a key when writing to the cache.
For a hypothetical GraphQL endpoint https://example.com/graphql
that you POST
to with a query in the body, if you went ahead with your approach, there would be a total of one entry in the cache with the URL key https://example.com/graphql
, and the response to that cached request would be used to satisfy all other POST
requests, regardless of what the query in the body is. That's almost certainly not going to lead to the expected behavior.
I know that @arackaf has explored the GraphQL + Workbox use case in the past, and I think the conclusion he came to was that using GET
requests, where the query is encoded in the URL and therefore part of the cache key, was the only viable approach. There's more context at https://github.com/GoogleChrome/workbox/issues/1620
Right. You absolutely do not have to POST to a graphql endpoint to query it—confirm for yourself by hitting it with a simple fetch. Most clients have a way to override the method, and if not, I'd recommend looking into alternate GraphQL clients.
@jeffposnick @arackaf The problem with converting graphql POSTs to GETs in general is that queries can be large, and overrun the maximum URL sizes allowed by some CDNs. We ran into this with Fastly. A more scalable solution is to continue to use POST, but add a hash of the query and parameters to the URL's query string. In this way the body can be ignored and the URL is sufficient for the cache key. This is very easy to do with apollo middleware, for example.
The approach to graphql caching aside, I think it should be up to the developer to determine if a POST request is idempotent and suitable for caching. Even if lower level browser APIs can't cache posts, as long as the developer provides a custom cache key, I think workbox should allow them to cache it. I feel it's in the spirit of the kind of fine-grained control over caching that workbox and service workers in general are meant to afford.
@markbrocato good point on the long queries problem - there are solutions to that, of course: https://github.com/arackaf/generic-persistgraphql
I've merged your PR, and it will be in the next pre-release of v6.
I think it's a good idea to perform the method check in the order in which your PR does it, but again, I don't want to advertise this as "Workbox now supports caching POST
requests."
thanks for the fix, i have the need for it in the context of a laravel app redirecting in a CRUD page to the entity index (list all) page after posting a new entity. my current sample code is
generateSW({
runtimeCaching: [
{
urlPattern: "https://pwa.test/post",
method: "POST",
handler: 'NetworkFirst',
options: {
cacheName: 'post',
backgroundSync: {
name: "myQueueName",
options: {
maxRetentionTime: 24 * 60
},
},
cacheKeyWillBeUsed : ???
},
}, ...
but i couldn't find out how to fill in the missing pieces instead of the ??? after looking into the test scenarios of the merge, anyone can shine a light on it?
When using runtimeCaching
in generateSW
, you can specify custom plugins via the options.plugins
array, along the lines of:
runtimeCaching: [{
urlPattern: "https://pwa.test/post",
method: "POST",
handler: 'NetworkFirst',
options: {
cacheName: 'post',
backgroundSync: {
name: "myQueueName",
options: {
maxRetentionTime: 24 * 60
},
},
plugins: [{
cacheKeyWillBeUsed: () => {
// You code to return a custom Request goes here.
}
}]
},
}]
Using GraphQL Persisted Queries allows you to pass the ID of the query you are requesting as a query parameter. The request body can thus be totally ignored for caching purposes. This is the main use case for allowing POST in Workbox today. Here is an example of my use case and the above workaround:
{
method: "POST",
urlPattern: ({ url }) =>
url
.toString()
.endsWith("/graphql?operationName=fetchClientVersion"),
handler: "StaleWhileRevalidate",
options: {
cacheName: "FeedVersion",
plugins: [
{
cacheKeyWillBeUsed: (param) => {
console.log("workbox TEST", param);
return Promise.resolve(param.request.url);
},
},
],
},
},
Library Affected: workbox-strategies
Browser & Platform: all browsers
Issue or Feature Request Description: Allow post caching
Developers should be able to cache
post
requests asget
requests usingcacheKeyWillBeUsed
. Currently there is a check that prevents post caching, which checks the initial request instead of the request returned bycacheKeyWillBeUsed
, which should be respected as a custom cache key.A related issue #2574 was filed but closed by the author.