Skip to content
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

Merged
merged 8 commits into from
Feb 10, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Feb 4, 2020

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's how you might remove a 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 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 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.

@benjamn benjamn added this to the Release 3.0 milestone Feb 4, 2020
@benjamn benjamn self-assigned this Feb 4, 2020
benjamn added a commit that referenced this pull request Feb 4, 2020
TODO This commit should be reverted before PR #5909 is merged.
@benjamn benjamn force-pushed the EntityStore-modify-method branch 2 times, most recently from 0a96ec6 to b4d53fa Compare February 5, 2020 19:09
@codecov

This comment has been minimized.

});

if (needToMerge) {
this.merge(dataId, changedFields);
Copy link
Member Author

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.

Comment on lines +2047 to +2053
cache.watch({
query: queryA,
optimistic: true,
immediate: true,
callback(data) {
aResults.push(data);
},
Copy link
Member Author

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!

benjamn added a commit that referenced this pull request Feb 7, 2020
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.
@benjamn benjamn mentioned this pull request Feb 7, 2020
3 tasks
Copy link
Member

@hwillson hwillson left a 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 ? {
Copy link
Member

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.
Follow-up to #5919 and 59d973e.

Now that the policies.rootTypenamesById check happens in store.get, we
don't have to rely on the getFieldValue wrapper to ensure __typename can
always be determined.
@benjamn benjamn merged commit 549262d into master Feb 10, 2020
benjamn added a commit that referenced this pull request Feb 10, 2020
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.
@3nvi
Copy link

3nvi commented Mar 13, 2020

@benjamn
I have successfully deleted the cache data using modify

client.cache.modify('ROOT_QUERY', {
          packScheduleForToday(_, { DELETE }) {
            return DELETE
          },
          currentActivePack(_, { DELETE }) {
            return DELETE
          },
        })

From logging the cache object, I can see that the data is gone.

Now how do I get the UI to re-render? I've tried with readQuery and watchQuery and it does not work.

As mentioned here cache.watch is triggered, but I am not sure how that is useful.

Do it inside an update within your mutation. You'll get the UI to update automatically

@peterlazar1993
Copy link

@benjamn
I have successfully deleted the cache data using modify

client.cache.modify('ROOT_QUERY', {
          packScheduleForToday(_, { DELETE }) {
            return DELETE
          },
          currentActivePack(_, { DELETE }) {
            return DELETE
          },
        })

From logging the cache object, I can see that the data is gone.
Now how do I get the UI to re-render? I've tried with readQuery and watchQuery and it does not work.
As mentioned here cache.watch is triggered, but I am not sure how that is useful.

Do it inside an update within your mutation. You'll get the UI to update automatically

Yes that worked. Missed it totally. 🤦‍♂
Do you reckon cache.evict works this way? Cause it does not look like it does.

Also what can I do if I want to remove something from the cache without using a mutation

@darkbasic
Copy link

darkbasic commented Mar 13, 2020

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 cache.evict and won't trigger any update unless you re-run the query (in such case it will trigger a refetch).

@benjamn
Copy link
Member Author

benjamn commented Mar 13, 2020

Hey everyone, my PR #6046 went out yesterday in @apollo/client@3.0.0-beta.40. Please give your cache.modify and cache.evict use cases another try, using that version!

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 mutate callback. The client still calls broadcastQueries following the same operations as it did before (such as mutate), but even those calls should be unnecessary now that the broadcast happens automatically, triggered by any modifications to the cache.

@darkbasic
Copy link

darkbasic commented Mar 13, 2020

@benjamn thanks for the update. I still didn't look at the code but did you expose isOptimistic inside update? Because otherwise there is no way to discern if it's an optimistic update or not and consequently call cache.modify with the correct optimistic parameter.

@darkbasic
Copy link

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:

Uncaught TypeError: Cannot read property 'getStore' of undefined
    at te.<anonymous> (RNDebuggerWorker.js:2)
    at te.s.emit (RNDebuggerWorker.js:2)
    at RNDebuggerWorker.js:2
    at RNDebuggerWorker.js:2

I think it has something to do with the Apollo extension because now it doesn't work as well as it did before.

@benjamn
Copy link
Member Author

benjamn commented Mar 13, 2020

@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.

@benjamn
Copy link
Member Author

benjamn commented Mar 13, 2020

it still doesn't trigger a UI re-render when I evict something from the cache

Diagnostic question: does the cache.evict(...) call return true or false? If it returns false, then nothing was evicted, so nothing will be rerendered.

@darkbasic
Copy link

Diagnostic question: does the cache.evict(...) call return true or false? If it returns false, then nothing was evicted, so nothing will be rerendered.

@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.

@bsunderhus
Copy link

I have the same situation @darkbasic. using cache.evict cleans the cache data but doesn't trigger an update on the queries that depended on the data that was evicted.

@benjamn
Copy link
Member Author

benjamn commented Mar 13, 2020

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.

@JamieS1211
Copy link

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 cache.modify to update the list of items. Is this now outdated advice?

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:

update: (cache, result) => {
    if (result.data !== undefined && result.data !== null) {
        cache.evict(`${result.data.updateActivity.__typename}:${result.data.updateActivity.id}`
        cache.gc()
    }
}
Example images

image
image

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.

@benjamn
Copy link
Member Author

benjamn commented Mar 16, 2020

Is this now outdated advice?

The cache.evict(id, fieldName?) method deletes the field (or the whole object, if only id is provided), whereas cache.modify(id, { fieldName(value) { return ... }}) modifies the fieldName in place. Those are different use cases, so both methods are still supported and recommended, as appropriate. Although it's possible to use cache.modify to delete fields, the equivalent cache.evict code is almost always shorter.

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!

@peterlazar1993
Copy link

Hey everyone, my PR #6046 went out yesterday in @apollo/client@3.0.0-beta.40. Please give your cache.modify and cache.evict use cases another try, using that version!

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 mutate callback. The client still calls broadcastQueries following the same operations as it did before (such as mutate), but even those calls should be unnecessary now that the broadcast happens automatically, triggered by any modifications to the cache.

@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.

https://codesandbox.io/s/billowing-voice-8g57d

@JamieS1211
Copy link

Hi @benjamn,

Thanks for the info regarding the identify method. Had a bit of trouble with it as it returns a Maybe<String> type and as cache.evict needs a string type it was a bit annoying - perhaps in a different issue we could discuss the returned value when identify cannot find an object / when evict is given null.

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.

@JamieS1211
Copy link

@benjamn dropped you an email with a link to download repo zip file on your email linked to github. Hope it helps with testing :)

@ryanwalters
Copy link

Hey @benjamn, this is great!

Does cache.modify handle nested fields as well?

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),
    };
  },
});

benjamn added a commit that referenced this pull request Apr 21, 2020
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/client@3.0.0-beta.34 by PR #5909, so it has never been out of beta
testing, so breaking changes are still fair game.
benjamn added a commit that referenced this pull request Apr 21, 2020
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/client@3.0.0-beta.34 by PR #5909, so it has never been out of beta
testing, so breaking changes are still fair game.
@benjamn
Copy link
Member Author

benjamn commented Apr 30, 2020

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.

benjamn added a commit that referenced this pull request May 16, 2020
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.
benjamn added a commit that referenced this pull request May 18, 2020
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.
benjamn added a commit that referenced this pull request May 18, 2020
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.
@darkbasic
Copy link

To everyone subscribed: this has probably gone unnoticed but one of the best features of Apollo 3 just got removed: #6289

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.