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

Make client.{reset,clear}Store pass appropriate discardWatches option to cache.reset #8873

Merged

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Sep 30, 2021

Follow-up to PR #8852, inspired by discussion #8872 started by @KeithGillette. Depending on the discussion, we may need to make additional changes to align the various store-resetting/clearing operations, so I've left this PR as a draft for now.

return this.cache.reset();
}

public resetStore(): Promise<ApolloQueryResult<any>[]> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The QueryManager#resetStore method is no longer needed, because ApolloClient#resetStore actually just calls client.queryManager.clearStore, and QueryManager is a private API.

Comment on lines +50 to +54
export function resetStore(qm: QueryManager<any>) {
return qm.clearStore({
discardWatches: false,
}).then(() => qm.reFetchObservableQueries());
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper function allows us to preserve existing tests that called qm.resetStore() (now removed).

@KeithGillette
Copy link
Contributor

KeithGillette commented Sep 30, 2021

Thanks for responding so quickly to my discussion #8872, @benjamn. In looking at these changes, it appears to me that If this PR is merged, both ApolloClient.resetStore() and ApolloClient.clearStore() will call the QueryManager.reset() with different parameter values and that QueryManager.reset() in turn calls cache.reset(). If that's true, I think that answers my question regarding the relationship of the APIs, which was only prompted by the mention in the CHANGELOG of changes to cache.reset()` from PR #8852.

As a consumer of ApolloClient, I'm really only concerned about the public API, so my bigger question regarding the use cases for resetStore vs. clearStore is still unresolved. As I said in the discussion, the documented recommendation to use resetStore on user sign out and have that method refetch active queries never made sense to me, as it ends up reloading data into the cache and can lead to errors on active queries requiring authorization. If clearStore cancels any active queries then clears the cache, it would seem to me that would be the best method to use on user sign out, with resetStore preferred for use in quickly clearing all cache contents within a user session.

I may be way off base on all of that, but I guess my recommendation is that this PR include documentation updates that clearly differentiate the appropriate use cases for resetStore vs. clearStore.

@benjamn benjamn marked this pull request as ready for review October 4, 2021 19:57
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.

Looks great to me @benjamn - thanks!

@benjamn benjamn force-pushed the align-resetStore-and-clearStore-with-cache.reset-discardWatches branch from e80dd03 to 3f07d95 Compare October 4, 2021 22:20
@benjamn benjamn merged commit 58ae8b8 into main Oct 4, 2021
@benjamn benjamn deleted the align-resetStore-and-clearStore-with-cache.reset-discardWatches branch October 4, 2021 22:29
@benjamn
Copy link
Member Author

benjamn commented Oct 4, 2021

The initial code changes from this commit are now available in @apollo/client@3.4.16 and @apollo/client@3.5.0-rc.1.

While I agree the docs could take a stronger stance on which method (resetStore or clearStore) is recommended after logout, we do at least mention both methods and their differences here and here.

My intuition is that resetStore is a good way to switch into a cache/UI state that approximates what you'd see if the page was initially loaded without authentication, whereas clearStore is more for situations where you expect the user to close the page and leave soon. Yes, there may be authentication errors with resetStore refetching queries, but if you're handling those errors gracefully, resetStore can be an easy way to re-render the logged-out UI.

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants