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

[3.0 Beta 41] Evict API does not support eviction based on arguments #6098

Closed
danReynolds opened this issue Mar 30, 2020 · 7 comments
Closed
Assignees
Milestone

Comments

@danReynolds
Copy link
Contributor

danReynolds commented Mar 30, 2020

Hello! I am playing around with the 3.0 eviction API. I see that we can evict by dataId and fieldName. If I make two queries for a user's total balance for their currencies, they get stored in the cache under the ROOT_QUERY like this:

totalBalance({ currency: 'USD' })
totalBalance({ currency: 'CAD' })

If they they added a new USD account, I want to invalidate their USD total balance by doing:

cache.evict('ROOT_QUERY', 'totalBalance({ currency: "USD" })')

But that isn't a valid field name, instead I need to do:

cache.evict('ROOT_QUERY', 'totalBalance')
But that invalidates both my cached queries. I would think that there should be a way to invalidate queries based on their arguments, since in some applications there may be many of the same query type made with different filters etc and that users can perform actions to invalidate subsets of these.

The issue is here: https://github.com/apollographql/apollo-client/blob/master/src/cache/inmemory/entityStore.ts#L130

it checks for modifiers based on the fieldName not the storeFieldName. It would be great if it could first check modifiers for the storeFieldName too, and have that modifier take precedence over the fieldName modifier. Thoughts?

@danReynolds danReynolds changed the title Evict API does not support eviction based on arguments [3.0 Beta 41] Evict API does not support eviction based on arguments Mar 30, 2020
@benjamn benjamn added this to the Release 3.0 milestone Apr 3, 2020
@benjamn benjamn self-assigned this Apr 3, 2020
@benjamn
Copy link
Member

benjamn commented Apr 3, 2020

Are you comfortable with having to pass exactly the same arguments that were originally used to store the field in the cache? The only reason I ask: I don't think we can reasonably match subsets of fields if you pass some but not all of the original arguments.

Of course, passing only an ID and a field name will continue to evict all variations of the field, regardless of arguments.

If that feels right, then I think we could extend the API with another argument:

cache.evict("ROOT_QUERY", "totalBalance", { currency: "USD" })

If I recall correctly, the reason I didn't add arguments to the cache.evict API originally was that the EntityCache did not have easy access to the Policies object, but that changed with commit 9dcc33c.

@danReynolds
Copy link
Contributor Author

danReynolds commented Apr 3, 2020

Got it yep that sounds good to me, thanks for looking into this. I can make that change and throw up a PR.

@Vincz
Copy link

Vincz commented Apr 17, 2020

Hey guys! Would it be possible to have some kind of better control on the cache eviction ?
I have a use case here https://github.com/apollographql/apollo-client/issues/6159

So, if I understand your PR correctly, it would allow to clear the cache based on a Query name + specific variables, but the variables should be an exact match, right ?

What if we wanted to have more control ? Let's say if I wanted to remove all queries totalBalance where the currency variable is set to USD no matter the values of the other variables ?

We could also use a callback, to check if a query should be clear or not :

cache.evict("ROOT_QUERY", "totalBalance", (variables, data) => {
    return variables.currency === 'USD' or variables.currency === 'EUR';
});

So, it would remove the cache where the currency is USD or EUR but not for CAD.
This way, we could handle any specific use case.

@danReynolds
Copy link
Contributor Author

danReynolds commented Apr 17, 2020

Hey guys! Would it be possible to have some kind of better control on the cache eviction ?
I have a use case here #6159

So, if I understand your PR correctly, it would allow to clear the cache based on a Query name + specific variables, but the variables should be an exact match, right ?

What if we wanted to have more control ? Let's say if I wanted to remove all queries totalBalance where the currency variable is set to USD no matter the values of the other variables ?

We could also use a callback, to check if a query should be clear or not :

cache.evict("ROOT_QUERY", "totalBalance", (variables, data) => {
    return variables.currency === 'USD' or variables.currency === 'EUR';
});

So, it would remove the cache where the currency is USD or EUR but not for CAD.
This way, we could handle any specific use case.

Yea so they're stored by stringified variables in the cache like this:

ROOT_QUERY: {
  totalBalance({ currency: x, filter: y, otherFilter: z })
}

and you may have a number of such cached queries. The variables aren't currently stored separately so you'd have to be parsing them for each cached totalBalance fieldName and then passing them to the predicate like you've supplied above. I'd like to hear @benjamn's thoughts, I assume the issue with that is performance concerns as well as some API design because this opens up the eviction API to conditional eviction. If it were to support a predicate it would probably be more performant to have it supply a readVariables kind of API similarly to how modify supplied a readField so that you only incur the parsing cost if your predicate needs that. I'm not sure myself on how I'd feel about it taking a function, I think that might expand the scope of the core API too much from a utility for eviction to a system of conditions for eviction, but I'd like to hear more from core contributors.

@Vincz
Copy link

Vincz commented Apr 17, 2020

Wouldn't it be better than a useless refetch anyway ? It's one of the mail goal of GraphQL. Fetch and refetch only what you need.
Usually, query variables are small objects, they shouldn't have a huge impact if stored or parsed or checked. And the callback function would be the developer responsibility, but also for small check, it shouldn't be a problem with performances.
And it would open a lot of needed flexibility !

@Vincz
Copy link

Vincz commented Apr 29, 2020

Hey @benjamn & @danReynolds !
Any update about this?
It seems that this feature is really needed (apollographql/apollo-feature-requests#4).

@benjamn
Copy link
Member

benjamn commented Jul 10, 2020

Since we now support evicting by specific arguments (see #6141 and #6364), I think this issue can be closed. Still, I like your idea of an eviction predicate function @Vincz. Can you open a separate feature request for that? Feel free to refer back to this discussion for background/context.

@benjamn benjamn closed this as completed Jul 10, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants