Query is not cleaned up after component is unmounted in afterware

[jacobk]

Intended outcome:

Queries should be removed when a component is unmounted as a result from logic in afterwares.

See #1195

Actual outcome:

Query is lingering in apollo-client after component is unmounted.

How to reproduce the issue:

• clone https://github.com/jacobk/react-apollo-error-template
• npm install
• npm start
• Open browser and console
• Navigate to localhost:3000/
• Notice that no queries are active (dev tools and console)
• Click "Trigger"
• This will render graphql wrapped route that will get 401. The afterware will navigate back to the index route, hence unmounting the wrapped component.
• Notice that the wrapped component is properly unmounted but the query is still in apollo-client

Demo

[calebmer]

As of 0.11.x we intentionally keep observable queries from unmounted components around. It means that a user’s updateQueries and reducers will still run even after the component has unmounted. Perhaps after we merge #1310 this decision can be reverted.

If I remember correctly this had the side-effect of making it difficult to for you to call resetStore, correct? Why is keeping queries around a bad thing for you, and can we fix that while keeping queries around?

[jacobk]

@calebmer oh, in that case it seems I've not managed to reproduce the error I had in my real app. Since downgrading my reproduction repos solves it indeed, which is not the case for my real app. Also, invoking resetStore seems to work pretty good in the repo I linked above.

The difficulty with resetStore was that it caused the query to be refetched. In my real app it causes further issue when I want to login a user again since I do resetStore upon login, and the query is still sitting in the client and is refetched again, triggered by the resetStore.

I need to dig deeper and try to isolate what I do differently in the real app.

[jacobk]

Still haven't manage to reproduce in the error-template project. But I noticed that if you add a client.resetStore after this line https://github.com/jacobk/apollo-issue-example/blob/master/app/graphql.js#L15 in my initial repro-repo it ends up in an infinite tailspin recursively fetching and failing.

EDIT

This commit reproduces the recursive refetch issue describe above in the react-apollo-error-template: jacobk/react-apollo-error-template@1f2fd60

I will continue to investigate and narrow in on my initial problem but the above is probably worth looking in to.

[jacobk]

The last version of https://github.com/jacobk/react-apollo-error-template is probably the same issue I'm seeing. The difference is that I don't invoke resetStore directly in my real app which has a state machine of sorts for auth which saves me from the infinite recursion.

[jacobk]

@calebmer I think the current issue might be this empty subscribe in react-apollo: https://github.com/apollographql/react-apollo/blame/master/src/graphql.tsx#L642

When the wrapped react component unmounts the empty observer added when recyling causes https://github.com/apollographql/apollo-client/blob/master/src/core/ObservableQuery.ts#L439-L441 to not teardown the query and hence it will refetch on resetStore even after the component is not mounted.

What is that subscribe needed for?

[jacobk]

I was playing around with the new fetchPolicy and doing

    observableQuery.setOptions({
      fetchPolicy: 'cache-only'
    });

before saving the query in the recycler seems to fix the issue. It would probably make sense to store the old policy for when its reused.

[calebmer]

@jacobk the reason we have that subscribe is so that updateQueries and refetchQueries will still run on queries for unmounted components so that when those components remount they have the latest data. This is intentional.

[jacobk]

@calebmer thanks for the answer!

I guess it's fair to assume that it's an unwanted side effect to also have the recycled queries be refetched when resetting the store.

This is part react-apollo and part apollo-client. I think my proposal with changing the fetchPolicy for recycled queries seems sensible for react-apollo but I'm not sure what other side effects it might have?

Regarding apollo-client I think I'm not alone in not fully understanding the intentions behind resetStore. I understand that there are apps where you might have scenarios for queries still active when you reset the store. Personally I thought resetStore was the means to get a clean slate and make sure that all queries and cached data was wiped - which I think is a common need in logout flows where the entire schema was behind auth.

[calebmer]

Personally I thought resetStore was the means to get a clean slate and make sure that all queries and cached data was wiped - which I think is a common need in logout flows where the entire schema was behind auth.

That was the point behind resetStore, and your proposed solution using fetchPolicy seems appropriate. Any unintended side effects are actually, likely, desired because I don’t think there is any time where we want queries in the recycler to fire a query with your network interface 😊

Want to submit a PR to react-apollo and we can build from there?

[abergenw]

I just wanted to add to the discussion that a working resetStore is quite important for my use case (meaning that recycled queries don't get fetched, apollographql/react-apollo/pull/531 ).

I like the mutation approach taken by GraphQL, making it possible for the client to update only data that has changed. However, there are situations where a lot of data is modified as a result of a mutation. Including all this data in the mutation response quickly becomes a pain in the ass. In many cases it is acceptable for the client empty the cache and refetch all active queries as a result of a successful mutation (resetStore).

Im currently working on an app using Relay and looking into Apollo as Relay in its current state offers very little support for "manually" manipulating the store. In addition to this the performance of Relay is terrible when a large amount of data is refetched (forceFetch).

[helfer]

Ok I think / hope this is now conclusively fixed with the standby option (only to be used for recycled queries). Please let me know if it's not the case.