-
Notifications
You must be signed in to change notification settings - Fork 830
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
Allow post caching #2615
Comments
… by returning a get request cache key from cacheKeyWillBeUsed.
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 If your use case is primarily motivated by performing some sort of fallback when a |
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 While modern CDNs might be able to use the For a hypothetical GraphQL endpoint 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 |
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 |
* Fixes #2615 by allowing developers to cache post requests by returning a get request cache key from cacheKeyWillBeUsed. * Validate effectiveRequest instead of the original request.
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 |
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.
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
|
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.
The text was updated successfully, but these errors were encountered: