Solve issue #7491 by supporting watchQueryOptions.refetchWritePolicy. #7810
Conversation
| if (process.env.NODE_ENV !== "production") { | ||
| if (process.env.NODE_ENV !== "production" && !context.overwrite) { |
There was a problem hiding this comment.
This change means you won't see Cache data may be lost... warnings when writing to the cache with overwrite: true, since we can assume overwriting is the behavior you want.
There was a problem hiding this comment.
I think the refresh approach is the way to go. We don't want to wait until 4.0 to get this functionality in, and even then changing the behavior of refetch could be jarring. I'm okay with refresh becoming part of our public API, especially knowing refetch is just wrapping refresh (so our maintenance burden isn't being increased by much).
Just wondering out loud (I know we talked about this but I can't remember if we thought of this combo). Did we consider a more explicit name like refetchAndOverwrite instead of refresh? That might make things a bit more clear.
On that note, we'll want to get this added into the API docs as well, here in particular. Thanks for working on this @benjamn!
By default, cache writes always overwrite existing field data, so this may not seem like a very consequential change. However, when a field has a merge function defined, the merge function will be called whenever incoming data is written to the field, receiving as its first two parameters (0) any existing data (possibly undefined) and (1) the incoming data. Since merge functions generally attempt to combine incoming data with existing data, when a merge function is defined, there is no easy way to tell the cache to _replace_ existing field data with incoming field data, instead of combining them. The overwrite:true option enables the replacement behavior by causing undefined to be passed as the existing data to any merge functions involved in the cache write, so the merge function behaves as if this was the first time any data had been written for the field in question, effectively replacing any existing data.
This is how I would ideally like to fix #7491, but I'm worried it could be a breaking change for any application code that relies on refetch results getting merged with existing data, rather than replacing it (which is what refetch was originally meant to do, since AC2). I will follow this commit with one that makes this the overwrite behavior opt-in (and thus backwards compatible).
The difference is that refresh overwrites existing fields by not passing existing data to field merge functions, whereas refetch passes existing data to merge functions, thereby combining it with the refetch result. If we continue down this path, we can say "the solution to #7491 is to switch from using refetch to using refresh," but that involves conscious effort by the developer, and would mean maintaining two very similar methods of the ObservableQuery class (refetch and refresh).
This reverts commit d87333b and introduces a new option for WatchQueryOptions called refetchPolicy. Because of the backwards compatibility concerns raised in #7810, the default setting for refetchPolicy needs to be "merge", though we hope developers will switch to refechPolicy:"overwrite" using the explicit option once Apollo Client v3.4 is released, to achieve the behavior I described in my comment #7491 (comment). We try to avoid introducing ad hoc new options, especially when we think one of the behaviors is preferable (overwriting, that is). However, since not all call sites of observableQuery.refetch are under the developer's control (for example, see refetchQueries after a mutation), it seemed important to have a way to _alter_ the behavior of refetch, rather than an alternative method (refresh). If one of these behaviors (hopefully "overwrite") becomes a clear favorite after Apollo Client v3.4 is released, we may either swap the default or remove the refetchPolicy option altogether in Apollo Client 4.
4e3b7eb to
fab36e3
Compare
This reverts commit d87333b and introduces a new option for WatchQueryOptions called refetchPolicy. Because of the backwards compatibility concerns raised in #7810, the default setting for refetchPolicy needs to be "merge", though we hope developers will switch to refetchPolicy: "overwrite" using the explicit option once Apollo Client v3.4 is released, to achieve the behavior I described in my comment #7491 (comment). We try to avoid introducing ad hoc new options, especially when we think one of the behaviors is preferable (overwriting, in this case). However, since not all observableQuery.refetch call sites are under the developer's control (for example, see refetchQueries after a mutation), it seemed important to have a way to _alter_ the behavior of refetch, rather than a new alternative method (refresh). If one of these behaviors ("merge" or "overwrite") becomes a clear favorite after Apollo Client v3.4 is released, we may either swap the default or remove the refetchPolicy option altogether in Apollo Client 4.
fab36e3 to
59f6ff0
Compare
I don't want the similarity of "refetchPolicy" and "fetchPolicy" to suggest a refetchPolicy is somehow a fetchPolicy for refetches. I think refetchWritePolicy better reflects purpose of this option, which is to configure how field data is written into the cache after a refetch.
I disagree with this approach, which means users would need to manually fix |
|
@Akryum We did it this way to avoid a breaking/surprising change in a minor release. We fully intend to make |
|
Well, since the default behavior is broken, it's technically a bug fix isn't it? |
|
@Akryum I mean, I would love to just fix this, but I am hesitant to release a new major version. What does your intuition as a maintainer say about how disruptive it would be to make |
|
I'm currently experiencing this because I'm migrating a big app from Apollo Client 2 to 3, that's why I'm saying it's a regression compared to the previous major. Looks like some other users are confused too (vuejs/apollo#1115, vuejs/apollo#1113 (comment), maybe vuejs/apollo#1026). IMO it should probably categorized as a bug/regression from v2 and bug fixing it without bumping the major version number would be fine. |
|
If we think about the semantics of |
|
As a side note, imo this is all made harder by the fact that |
After discussions in PR #7810 and with the client team, I/we believe the benefit of restoring the Apollo Client 2.x refetch behavior (that is, refetches overwrite any existing data), for the specific case of custom `merge` functions, outweighs the risk of breaking applications that depended on refetched results getting merged with existing results, especially since those applications can recover the previous AC3 behavior by passing refetchWritePolicy: "merge" on a per-query basis, or as a client-wide default using defaultOptions.watchQuery: new ApolloClient({ defaultOptions: { watchQuery: { refetchWritePolicy: "merge", }, }, })
|
Wouldn't you want this same behavior when remounting a query with |
As I explained in #7491 (comment), I believe the best way to solve #7491 is to allow writing to the cache in a way that invokes any
mergefunctions with undefinedexistingdata, thereby replacing the field value while still respecting the contract of themergefunction.I also believe this
overwrite: truebehavior should be the default behavior of theObservableQuery#refetchmethod, but I worry that switching the behavior back in a minor version (Apollo Client v3.4) would be a breaking change for any application code that intentionally or accidentally relies onrefetchresults gettingmerged withexistingdata, rather than replacingexistingdata (which is whatrefetchwas originally meant to do, since AC2).If we're not comfortable with fixing/breakingrefetchin this way, then we need to make the new behavior optional somehow. My best idea right now is to add a newObservableQuerymethod,refresh, which is just likerefetchexcept that it writes data to the cache withoverwrite: true. I don't love this idea, so I've split it out in to its own commit. If we think we can do without this extra backwards-compatibility measure, my preference would be to revert/remove this commit. I'm curious to hear anyone's (but especially @hwillson and @jcreighton's) thoughts on this question.Update: see #7810 (comment) and dfc2609 for a better approach, involving a new
refetchPolicyoption. This approach should be completely backwards compatible because it requires explicitly passingrefetchPolicy: "overwrite"to achieve the overwriting behavior.For this PR, I recommend reviewing commit-by-commit, reading the commit messages for details.