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

Cache not updated if there's no active subscriber to a query #1068

Closed
mruf opened this issue Dec 20, 2016 · 12 comments
Closed

Cache not updated if there's no active subscriber to a query #1068

mruf opened this issue Dec 20, 2016 · 12 comments

Comments

@mruf
Copy link

mruf commented Dec 20, 2016

I have the following scenario:

User navigates to a screen with a list of Foo items
-> I call watchQuery() on the query to retrieve the items, subscribe to the Observable and render the results.
User moves to a different screen
-> I unsubscribe from the Observable.
User creates a new Foo item
-> I create a mutation to create the item and use updateQueries to update the Foo items query
User returns to the screen with the Foo items list
-> I subscribe again to the Foo items query using watchQuery()

Intended outcome: List of Foo items with the new item
Actual outcome: Stale list of Foo items with the new item missing

What I've found out is that the cache does not seem to be updated if there are no active subscribers to a query effectively rendering updateQueries useless in this case.

We are using [email protected] together with Aurelia.
The behavior I described above worked as expected in 0.5.1 so it must be a recent change.

@mruf
Copy link
Author

mruf commented Dec 20, 2016

I also have a problem with using query() instead of watchQuery() which might be related to the issue above.
If I update the results of a query after a mutation using updateQueries, query() does not return the new item as part of the results when querying for the same list again. It returns the stale list without the new item.
I checked the store and the newly created item is there after the mutation. I am using dataIdFromObject.

@helfer
Copy link
Contributor

helfer commented Dec 20, 2016

@mruf: yeah, this is a recent change that fixed a bug we had (inactive queries weren't removed).

I think a better way to update the cache is needed, because it's not reasonable to run update queries for any query that was ever run, but it's also not reasonable to not be able to update the cache because I think your use-case is pretty common.

On the spectrum from "do it yourself" all the way to "it just works" we're currently closer to the do-it-yourself side of things. I don't think we want to go all the way to "it just works" because that would have implications for flexibility, complexity and performance, but we should do our best to come up with something that is more intuitive to use while still remaining flexible and performant.

PS: I was going to say that in your case the mutation should apply the update to the cache, but that obviously doesn't work any more either, because updateQueries only affects queries that are currently active.

@yurigenin
Copy link

At the end of #903 I kind of anticipated this would happen. I think the way to make it work 'automatically' without relying on developers is to support registering updateQueries callbacks on ApolloClient itself. It could be a generic callback called for all query results (optimistic and not) and it would make it independent of all queries and mutations. What do you think? The callback would take mutation name, name of query in question, variables, current data in cache and would return new data.

@calebmer
Copy link
Contributor

I’m going to close this as there is more information on this issue in #903. But I do agree with @helfer. For now at least this should be a “do it yourself” thing if it is that important. GitHunt “solves” this problem by using forceFetch as @yurigenin noted in #903, so that’s an option. Another option is leaving queries active where you know a user might need them again. So this would mean not unmounting React components if you think a user may return.

I agree it’s not optimal, but until we find a better solution it’s good to do this yourself. We’ll be doing some refactors soon that might address this so stay tuned 👍

@yurigenin
Copy link

@calebmer @helfer Can you please elaborate what you mean by 'do it yourself'? Apollo store is opaque to me as an apollo client user. How do I actually update the store myself when the mutation results come back? I could do it in the then function of the promise returned by the mutate method but exactly how? updateQueries gives me a nice abstraction where it patches the store behind the scenes for me. If I have to do it myself I currently do not have means to do it.

I follow the updateQuery developments and @helfer mentioned several times that he is about to 'fix' it so that we can update results of inactive queries. How far are you guys from that ultimate fix?

Thank you!

@calebmer
Copy link
Contributor

@yurigenin we mean don’t unsubscribe from queries you want to update. So if you are using React, for instance, this means keeping the containers mounted that you want to update but hiding the children. We agree that this is not ideal, but it is a temporary workaround.

@yurigenin
Copy link

@calebmer Most of the people use the react-router package and not in a position to override how it deals with mounting/dismounting of components when the routes change. I hope you agree with me.

So, my question is still out there: when do you think we can expect a solution for this issue?

@helfer
Copy link
Contributor

helfer commented Jan 22, 2017

@yurigenin we already have a rough design, which we should be able to implement within less than a week.

@willdeuschle
Copy link

#903 led me here. Any update on this or what that rough design might be @helfer? I would be interesting in helping solve this problem as it is a pain point for me as well, but don't want to dive in cold if there is already something in the works.

@helfer
Copy link
Contributor

helfer commented Feb 5, 2017

@willdeuschle We're going in the direction of #1224, but first we'll implement an imperative way to update the cache, that is a read and a write function for the store, which lets you update it in whichever way you want in response to a query. That should enable all use-cases for a start, and will let us (and anyone else who wants!) implement some nice declarative APIs on top of it (like updateQueries). 🙂

@mlukaszczyk
Copy link

@helfer this sounds great! Any hints on when we can expect this new feature (:?

@helfer
Copy link
Contributor

helfer commented Feb 10, 2017

@mlukaszczyk We've started working on it in #1280. If I have time I'll write a store write implementation tomorrow, and then integrate it with mutations. If all goes according to plan, we can release it next week.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 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

6 participants