-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Implement InMemoryCache#modify for surgically transforming fields. #5909
Conversation
TODO This commit should be reverted before PR #5909 is merged.
0a96ec6
to
b4d53fa
Compare
This comment has been minimized.
This comment has been minimized.
b4d53fa
to
bd8e958
Compare
}); | ||
|
||
if (needToMerge) { | ||
this.merge(dataId, changedFields); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was very happy to be able to implement this last portion of store.modify
using store.merge
, so that there's no longer any redundancy between modify
, merge
, and delete
.
cache.watch({ | ||
query: queryA, | ||
optimistic: true, | ||
immediate: true, | ||
callback(data) { | ||
aResults.push(data); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had forgotten that cache.watch
does not immediately deliver an initial result. This immediate: true
option that I added seems pretty useful!
I referred to cache.writeData as a "foot-seeking missile" in PR #5909, because it's one of the easiest ways to turn your faulty assumptions about how the cache represents data internally into cache corruption. PR #5909 introduced an alternative api, cache.modify(id, modifiers), which aims to take the place of the more "surgical" uses of cache.writeData. However, as you can see, in almost every case where cache.writeData was used in our tests, an appropriate query was already sitting very close by, making cache.writeQuery just as easy to call. If you think your life is worse now that you have to pass a query to cache.writeQuery or a fragment to cache.writeFragment, please realize that cache.writeData was dynamically creating a fresh query or fragment behind the scenes, every time it was called, so it was actually doing a lot more work than the equivalent call to cache.writeQuery or cache.writeFragment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am REALLY looking forward to the headache, boilerplate and wasted cycles this is going to reduce. Really great stuff @benjamn! I can't wait to get this out in the next beta
, so LGTM!
// fields of that entity whose names match fieldName according to the | ||
// fieldNameFromStoreName helper function. | ||
public delete(dataId: string, fieldName?: string) { | ||
return this.modify(dataId, fieldName ? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely very cool that you were able to re-implement delete
using modify
! Just a side note / reminder for us - when we document modify
we'll want to highlight delete
as an option as well (since using delete
might be easier for people to remember than using the DELETE
sentinel with modify
).
…ted. I needed this functionality for some tests of cache.modify, but I think I can expand it into a version of cache.watch that returns an async iterator if you do not supply a callback.
The cache.writeQuery and cache.writeFragment methods do a great job of adding data to the cache, but their behavior can be frustrating when you're trying to remove specific data from a field. The typical cycle of reading data, modifying it, and writing it back into the cache does not always simply replace the old data, because it may trigger custom merge functions which attempt to combine incoming data with existing data, leading to confusion. For cases when you want to apply a specific transformation to an existing field value in the cache, we are introducing a new API, cache.modify(id, modifiers), which takes an entity ID and an object mapping field names to modifier functions. Each modifier function will be called with the current value of the field, and should return a new field value, without modifying the existing value (which will be frozen in development). For example, here is how you might remove a particular Comment from a paginated Thread.comments array: cache.modify(cache.identify(thread), { comments(comments: Reference[], { readField }) { return comments.filter(comment => idToRemove !== readField("id", comment)); }, }); In addition to the field value, modifier functions receive a details object that contains various helpers familiar from read/merge functions: fieldName, storeFieldName, isReference, toReference, and readField; plus a sentinel object (details.DELETE) that can be returned to delete the field from the entity object: cache.modify(id, { fieldNameToDelete(_, { DELETE }) { return DELETE; }, }); As always, modifications are applied to the cache in a non-destructive fashion, without altering any data previously returned by cache.extract(). Any fields whose values change as a result of calling cache.modify invalidate cached queries that previously consumed those fields. As evidence of the usefulness and generality of this API, I was able to reimplement cache.delete almost entirely in terms of cache.modify. Next, I plan to eliminate the foot-seeking missile known as cache.writeData, and show that cache.modify can handle all of its use cases, too.
0c231d1
to
6fdab9d
Compare
I referred to cache.writeData as a "foot-seeking missile" in PR #5909, because it's one of the easiest ways to turn your faulty assumptions about how the cache represents data internally into cache corruption. PR #5909 introduced an alternative api, cache.modify(id, modifiers), which aims to take the place of the more "surgical" uses of cache.writeData. However, as you can see, in almost every case where cache.writeData was used in our tests, an appropriate query was already sitting very close by, making cache.writeQuery just as easy to call. If you think your life is worse now that you have to pass a query to cache.writeQuery or a fragment to cache.writeFragment, please realize that cache.writeData was dynamically creating a fresh query or fragment behind the scenes, every time it was called, so it was actually doing a lot more work than the equivalent call to cache.writeQuery or cache.writeFragment.
Do it inside an |
Yes that worked. Missed it totally. 🤦♂ Also what can I do if I want to remove something from the cache without using a mutation |
The point is not using cache.modify instead of cache.evict: they both don't trigger a re-render if you delete a cache item. update(cache, {data: mutationData}) {
if (mutationData) {
const removedMatchId = mutationData.removeMatch;
cache.modify('ROOT_QUERY', {
matches: (matches: Reference[], helpers) => {
const removedMatchRef = helpers.toReference({
__typename: 'Match',
id: removedMatchId,
});
return matches.filter(({__ref}) => __ref !== removedMatchRef.__ref);
},
});
}
}, The previous code triggers the re-render because I'm updating the query itself: it's basically the same as a read/writeQuery but without the variables. update(cache, {data: mutationData}) {
if (mutationData) {
const cacheId = cache.identify({__typename: 'Match', id: mutationData.removeMatch});
cache.modify(cacheId, (_, {DELETE}) => DELETE);
}
}, This instead is the equivalent of |
Hey everyone, my PR #6046 went out yesterday in The most important commit in that very long sequence of commits was 9148394, which means any changes in the cache that affect watched queries will be automatically broadcast, allowing UI components that are observing those queries to rerender, without relying on the special behavior of the |
@benjamn thanks for the update. I still didn't look at the code but did you expose |
I tried latest version, but unfortunately it still doesn't trigger a UI re-render when I evict something from the cache. Also now I get the following when I start the application up:
I think it has something to do with the Apollo extension because now it doesn't work as well as it did before. |
@darkbasic For the purposes of debugging this issue, I would recommend disabling the devtools extension for now. We're going to focus on devtools much more after AC3 has been released. |
Diagnostic question: does the |
@benjamn I didn't check which value it returned, but I looked at the cache with the Apollo extension and it did get evicted indeed. Also if I navigate to another view and then back to the previous one it refetches the result from the network, which doesn't happen without the evict. |
I have the same situation @darkbasic. using |
Ok, I have a local reproduction, so I think I can get to the bottom of this. I'll let you know when there's something new to try, and if that doesn't work then I'll probably ask you for a reproduction. |
Hi there, I am having issues using beta 41 with react specifically to make a render update occur when using "cache.evict" to remove an item (for example on a mutation that causes a deletion). I have been browsing around and round this thread where you recommended updating using Also how should this interact with different types? For example I have two data types, listing and activities. Listings have a number of activities. In my case I am querying listings however mutating activities. I am assuming this is fine as the cache is a global store but thought it may still be worth mentioning. My update function looks like this:
I have validated that the evict function returns "true" however I have noticed it does this EVERY time even after I have evicted the object once so not sure if there is an issue here? Let me know your thoughts and if there is any other info I can supply. Sidenote: I have been testing this on a repo that has a minimal amount of code for this (was for a code test) and uses graphQL code generator & typescript (may be interesting). If you would like another project to test things out I can pop it on code sandbox or similar. |
The By the way, this won't solve the problem, but it will shorten your code: // Use cache.identify to avoid having to compute the ID yourself:
cache.evict(cache.identify(result.data.updateActivity)); Please do put a reproduction together if you have time! |
@benjamn As per this comment, my understanding is that cache operations can be done outside of the update callback passed to the mutation and they will update any queries watching. But it does not work as expected. Please see the reproduction here. I was expecting evicting and running gc would cause the readQuery call to update. |
Hi @benjamn, Thanks for the info regarding the identify method. Had a bit of trouble with it as it returns a The codesandbox from @peterlazar1993 is great and demonstrates the issue. I have already prepared the repo I used for testing and it is a bit more "real world" so I will get in contact with you directly to pass it over. |
@benjamn dropped you an email with a link to download repo zip file on your email linked to github. Hope it helps with testing :) |
Hey @benjamn, this is great! Does I tried something like this (which didn't work): cache.modify(cache.identify(getQuestion), {
answerConnection: {
answers: (answers, { readField }) => answers.filter((answer) => readField('id', answer) !== deleteAnswer.id);
},
}); Ended up with this (works! but more verbose): cache.modify(cache.identify(getQuestion), {
answerConnection: (answerConnection, { readField }) => {
return {
...answerConnection,
answers: answerConnection.answers.filter((answer) => readField('id', answer) !== deleteAnswer.id),
};
},
}); |
Most importantly, this change allows callers of cache.modify to avoid passing "ROOT_QUERY" as the ID for modifications of root Query data. Making the dataId parameter optional and moving it after the modifiers parameter felt much more idomatic and understandable than than trying to support two conflicting type signatures for the modify method, even though that extra effort could have avoided the breaking change. Please remember that the cache.modify API was added in @apollo/[email protected] by PR #5909, so it has never been out of beta testing, so breaking changes are still fair game.
Most importantly, this change allows callers of cache.modify to avoid passing "ROOT_QUERY" as the ID for modifications of root Query data. Making the dataId parameter optional and moving it after the modifiers parameter felt much more idiomatic and understandable than than trying to support two conflicting type signatures for the modify method, even though that extra effort could have avoided the breaking change. Please remember that the cache.modify API was added in @apollo/[email protected] by PR #5909, so it has never been out of beta testing, so breaking changes are still fair game.
I believe most/all of the automatic updating issues will be solved by #6221. Check it out if you have a few minutes/hours! Otherwise, I will comment again here when the changes have been released in a new beta version. |
The `cache.modify` API was first introduced in #5909 as a quick way to transform the values of specific existing fields in the cache. At the time, `cache.modify` seemed promising as an alternative to the `readQuery`-transform-`writeQuery` pattern, but feedback has been mixed, most importantly from our developer experience team, who helped me understand why `cache.modify` would be difficult to learn and to teach. While my refactoring in #6221 addressed concerns about broadcasting `cache.modify` updates automatically, the bigger problem with `cache.modify` is simply that it requires knowledge of the internal workings of the cache, making it nearly impossible to explain to developers who are not already Apollo Client 3 experts. As much as I wanted `cache.modify` to be a selling point for Apollo Client 3, it simply wasn't safe to use without a firm understanding of concepts like cache normalization, references, field identity, read/merge functions and their options API, and the `options.toReference(object, true)` helper function (#5970). By contrast, the `readQuery`-transform-`writeQuery` pattern may be a bit more verbose, but it has none of these problems, because these older methods work in terms of GraphQL result objects, rather than exposing the internal data format of the `EntityStore`. Since `cache.modify` was motivated to some extent by vague power-user performance concerns, it's worth noting that we recently made `writeQuery` and `writeFragment` even more efficient when rewriting unchanged results (#6274), so whatever performance gap there might have been between `cache.modify` and `readQuery`/`writeQuery` should now be even less noticeable. One final reason that `cache.modify` seemed like a good idea was that custom `merge` functions can interfere with certain `writeQuery` patterns, such as deleting an item from a paginated list. If you run into trouble with `merge` functions running when you don't want them to, we recommend calling `cache.evict` before `cache.writeQuery` to ensure your `merge` function won't be confused by existing field data when you write your modified data back into the cache.
The `cache.modify` API was first introduced in #5909 as a quick way to transform the values of specific existing fields in the cache. At the time, `cache.modify` seemed promising as an alternative to the `readQuery`-transform-`writeQuery` pattern, but feedback has been mixed, most importantly from our developer experience team, who helped me understand why `cache.modify` would be difficult to learn and to teach. While my refactoring in #6221 addressed concerns about broadcasting `cache.modify` updates automatically, the bigger problem with `cache.modify` is simply that it requires knowledge of the internal workings of the cache, making it nearly impossible to explain to developers who are not already Apollo Client 3 experts. As much as I wanted `cache.modify` to be a selling point for Apollo Client 3, it simply wasn't safe to use without a firm understanding of concepts like cache normalization, references, field identity, read/merge functions and their options API, and the `options.toReference(object, true)` helper function (#5970). By contrast, the `readQuery`-transform-`writeQuery` pattern may be a bit more verbose, but it has none of these problems, because these older methods work in terms of GraphQL result objects, rather than exposing the internal data format of the `EntityStore`. Since `cache.modify` was motivated to some extent by vague power-user performance concerns, it's worth noting that we recently made `writeQuery` and `writeFragment` even more efficient when rewriting unchanged results (#6274), so whatever performance gap there might have been between `cache.modify` and `readQuery`/`writeQuery` should now be even less noticeable. One final reason that `cache.modify` seemed like a good idea was that custom `merge` functions can interfere with certain `writeQuery` patterns, such as deleting an item from a paginated list. If you run into trouble with `merge` functions running when you don't want them to, we recommend calling `cache.evict` before `cache.writeQuery` to ensure your `merge` function won't be confused by existing field data when you write your modified data back into the cache.
The `cache.modify` API was first introduced in #5909 as a quick way to transform the values of specific existing fields in the cache. At the time, `cache.modify` seemed promising as an alternative to the `readQuery`-transform-`writeQuery` pattern, but feedback has been mixed, most importantly from our developer experience team, who helped me understand why `cache.modify` would be difficult to learn and to teach. While my refactoring in #6221 addressed concerns about broadcasting `cache.modify` updates automatically, the bigger problem with `cache.modify` is simply that it requires knowledge of the internal workings of the cache, making it nearly impossible to explain to developers who are not already Apollo Client 3 experts. As much as I wanted `cache.modify` to be a selling point for Apollo Client 3, it simply wasn't safe to use without a firm understanding of concepts like cache normalization, references, field identity, read/merge functions and their options API, and the `options.toReference(object, true)` helper function (#5970). By contrast, the `readQuery`-transform-`writeQuery` pattern may be a bit more verbose, but it has none of these problems, because these older methods work in terms of GraphQL result objects, rather than exposing the internal data format of the `EntityStore`. Since cache.modify was motivated to some extent by vague power-user performance concerns, it's worth noting that we recently made writeQuery and writeFragment even more efficient when rewriting unchanged results (#6274), so whatever performance gap there might have been between cache.modify and readQuery/writeQuery should now be even less noticeable. One final reason that `cache.modify` seemed like a good idea was that custom `merge` functions can interfere with certain `writeQuery` patterns, such as deleting an item from a paginated list. If you run into trouble with `merge` functions running when you don't want them to, we recommend calling `cache.evict` before `cache.writeQuery` to ensure your `merge` function won't be confused by existing field data when you write your modified data back into the cache.
To everyone subscribed: this has probably gone unnoticed but one of the best features of Apollo 3 just got removed: #6289 |
The
cache.writeQuery
andcache.writeFragment
methods do a great job of adding data to the cache, but their behavior can be frustrating when you're trying to remove specific data from a field. The typical cycle of reading data, modifying it, and writing it back into the cache does not always simply replace the old data, because it may trigger custommerge
functions which attempt to combineincoming
data withexisting
data, leading to confusion.For cases when you want to apply a specific transformation to an existing field value in the cache, we are introducing a new API,
cache.modify(id, modifiers)
, which takes an entity ID and an object mapping field names to modifier functions. Each modifier function will be called with the current value of the field, and should return a new field value, without modifying the existing value (which will be frozen in development).For example, here's how you might remove a
Comment
from a paginatedThread.comments
array:In addition to the field value, modifier functions receive a
details
object that contains various helpers familiar fromread
/merge
functions:fieldName
,storeFieldName
,isReference
,toReference
, andreadField
; plus a sentinel object (details.DELETE
) that can be returned to delete the field from the entity object:As always, modifications are applied to the cache in a non-destructive fashion, without altering any data previously returned by
cache.extract()
. Any fields whose values change as a result of callingcache.modify
will trigger invalidation of cached queries that previously consumed those fields.As evidence of the usefulness and generality of this API, I was able to reimplement
cache.delete
almost entirely in terms ofcache.modify
. Next, I plan to eliminate the foot-seeking missile known ascache.writeData
, and show thatcache.modify
can handle all of its use cases, too.