Allow toggle of cache policy used in 'perform' method #1521
Conversation
Sources/Apollo/ApolloClient.swift
Outdated
| } | ||
| return self.networkTransport.send( | ||
| operation: mutation, | ||
| cachePolicy: publishResultToStore ? .fetchIgnoringCacheData : .fetchIgnoringCacheCompletely, |
There was a problem hiding this comment.
@designatednerd I went back and forth on this, as I can see both .returnCacheAndFetch and .fetchIgnoringCacheData being expected behaviors by "default".
I'm willing to revert this to .default, but I wanted your take on this.
There was a problem hiding this comment.
I would say .default since for mutations this doesn't actually hit the cache on the way out.
There was a problem hiding this comment.
Oh right, I glossed over that part and didn't piece it together in my head.
Though, that still doesn't seem right to rely on the interceptor chain setup given that devs might be using their own caching implementation rather than the LegacyCacheRead that Apollo provides.
Then again... that's up to them to properly handle everything that they want to do anyway...
I'll just revert this portion to use .default like we agreed 😅
There was a problem hiding this comment.
Ahhh fair point - I think at some point I'd had the load on ApolloStore set up to throw an error if you tried to call it on anything other than a query (it was previously restricted to things that conformed to the GraphQLQuery protocol but that wound up being unworkable with the current setup), but it looks like that got moved up to the intereceptor. I'll chat with @martijnwalraven on that one.
There was a problem hiding this comment.
Yeah, cachePolicy was originally meant to only apply to queries, because the semantics don't really make sense for mutations (or subscriptions for that matter). I think we'll want to revisit cache policies once we have a better idea about the interaction between the request chain and caching, maybe that will allow us to differentiate between operation types again. That would also be the time to think about more flexible error handling, taking TTL into account, etc. So for now, I don't think what we do here really matters, as long as it means the default chain respects publishResultToStore.
Sources/Apollo/ApolloClient.swift
Outdated
| } | ||
| return self.networkTransport.send( | ||
| operation: mutation, | ||
| cachePolicy: publishResultToStore ? .fetchIgnoringCacheData : .fetchIgnoringCacheCompletely, |
There was a problem hiding this comment.
I would say .default since for mutations this doesn't actually hit the cache on the way out.
Mordil
force-pushed
the
mutation-cache-policy
branch
2 times, most recently
from
e5810b0 to
141aaab
Compare
|
@designatednerd I added a unit test, and also a query for When writing the unit test I thought I'd need it, but it turns out I didn't. I figured I'd leave it in anyway since I did the work to add it |
|
@Mordil If you don't mind removing that query that'd be great - trying to keep that API file from having too much stuff we're not actually using. |
Motivation: In certain circumstances it makes sense for developers to have full control over their cache data, and when performing mutations they do not want Apollo to automatically publish the results to the store. `LegacyCacheWriteInterceptor` properly handles this `CachePolicy` case, but `ApolloClient.perform` conformance uses `CachePolicy.default` with no configuration possibility. Modifications: - Change: `ApolloClientProtocol.perform` to accept a new `publishResultToStore` parameter - Change: `ApolloClient` to inspect `publishResultToStore` to determine which `CachePolicy` to use. Result: Developers should now have the ability to toggle the cache behavior of 'perform' to ignore the cache completely. This resolves apollographql#777
Mordil
force-pushed
the
mutation-cache-policy
branch
from
141aaab to
a729c0b
Compare
|
@designatednerd Done! |
Mordil
force-pushed
the
mutation-cache-policy
branch
from
a729c0b to
4e58b4a
Compare
Mordil
force-pushed
the
mutation-cache-policy
branch
from
4e58b4a to
a17a189
Compare
|
So what's left on this PR? It looks like from my perspective it's in a good enough shape? |
Motivation:
In certain circumstances it makes sense for developers to have full control over their cache data, and when performing mutations they do not want Apollo to automatically publish the results to the store.
LegacyCacheWriteInterceptorproperly handles thisCachePolicycase, butApolloClient.performconformance usesCachePolicy.defaultwith no configuration possibility.Modifications:
ApolloClientProtocol.performto accept a newpublishResultToStoreparameterApolloClientto inspectpublishResultToStoreto determine whichCachePolicyto use.Result:
Developers should now have the ability to toggle the cache behavior of 'perform' to ignore the cache completely.
This resolves #777