apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.38k stars 2.66k forks source link

resetStore throws for queries #10447

Open wmertens opened 1 year ago

wmertens commented 1 year ago

Issue Description

Very similar to this one issue but since there's a comment there

This should no longer be an issue in @apollo/client@latest - let us know otherwise, thanks!

and I'm still seeing this on latest client, I'm opening this new issue.

This is basically the repro code: await logoutMutation(); await client.resetStore()

The resetStore() immediately fires off new queries for the ones that are open, and they fail because of not being logged in. Instead of updating the useQuery() hooks they are created from, with error attributes, these failures are thrown as part of resetStore().

IMHO this is a bug

Link to Reproduction

https://codesandbox.io/s/brave-leavitt-3hzlm5

Reproduction Steps

Click the logout button. It will cause the query error to be uncaught

bignimbus commented 1 year ago

Hi @wmertens 👋🏻 thanks for opening this issue! Can you help me understand why clearStore isn't suitable for your needs? Just trying to establish a better understanding of your use case here.

wmertens commented 1 year ago

Actually I didn't know that existed, but in any case resetStore seems like the more correct thing to do, and the queries throwing as a result of it seems like a bug...

jerelmiller commented 1 year ago

Hey @wmertens 👋 !

Would you be willing to dive into more detail here?

resetStore seems like the more correct thing to do

It's not clear to me why this would be the case. This is likely very app-dependent but I'd like to better understand your position here and why you consider this the correct thing to do.

the queries throwing as a result of it seems like a bug

Could you also explain a bit more in detail here what you're expecting? What about queries throwing errors returned from a server do you find buggy?

github-actions[bot] commented 1 year ago

We're closing this issue now but feel free to ping the maintainers or open a new issue if you still need support. Thank you!

wmertens commented 1 year ago

@jerelmiller sorry, lost track of this.

Resetstore after logout ensures that no personal information is still visible by forcing all active queries to re-run without credentials.

As for the throwing, in the context of resetStore() that's not ok, because the calls that throw are "owned" by their initiators, not by the function calling resetStore. Instead, the errors should be returned in the hook responses as would normally happen.

jerelmiller commented 1 year ago

@wmertens apparently it was my turn to lose track of this 😆. Apologies!

To clarify, when you say:

in the context of resetStore()

Are you saying that resetStore() itself throws when there are errors?

And that being the case, you're saying that resetStore shouldn't throw itself, but rather the areas that "own" the queries should be responsible for this (i.e. each useQuery call site) correct?


I'll have to take this back to the team to figure out if this is behavior we'd want to change. I think there is value is knowing whether the refetched queries were successfully refetched or not (hence the exception). While resetStore is valuable for clearing out your store, it can also be useful if you want to full on clear the cache and allow all your queries to repopulate the cache from scratch to clear out "old" values.

Is there a reason a simple try/catch around your resetStore call wouldn't work in your case?

github-actions[bot] commented 1 year ago

We're closing this issue now but feel free to ping the maintainers or open a new issue if you still need support. Thank you!

wmertens commented 1 year ago

And I didn't see your response this time @jerelmiller ;)

No, catching resetStore is not ok because then the original queries don't throw and they don't update their calling components.

useQuery should update the component with error as soon as the resetStore causes it to refetch and fail.

melville commented 4 months ago

I am running into this issue with @apollo/client version 3.10.8, and I was wondering if there is any recommended workaround or plans for a fix.

My use case is the one called out in the documentation for resetStore: I want to clear all cached data and refetch queries after clearing auth tokens (when the user clicks a button to log out). The behavior I see is mostly as wmertens describes: resetStore throws the expected auth error. I can catch the error from resetStore, and the query does get refetched, but the result that comes back via the useQuery hook in my component includes the error AND the previously fetched data. This is true even when I explicitly set errorPolicy: 'none' for the query, and even if I await a clearStore before the call to resetStore. It seems like the cache is not getting reset before the query is refetched, and the query not respecting the errorPolicy.

I'm new to Apollo client but not to React, so if it seems like there is something wrong with my approach please let me know. I'm just trying to do the obvious thing as described in the documentation for resetStore.