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

GraphQLQueryWatcher doesn't update its dependent keys #1365

Closed
teodorpenkov opened this issue Aug 20, 2020 · 6 comments · Fixed by #1375
Closed

GraphQLQueryWatcher doesn't update its dependent keys #1365

teodorpenkov opened this issue Aug 20, 2020 · 6 comments · Fixed by #1375
Labels
caching question Issues that have a question which should be addressed
Milestone

Comments

@teodorpenkov
Copy link

With the 0.23.0 release Apollo changed the way it stores keys in the QueryWatcher https://github.com/apollographql/apollo-ios/blob/main/Sources/Apollo/GraphQLQueryWatcher.swift#L73-L83

The problem with this is that the watcher doesn't update its dependentKeys like it should. There is a commit that does that but I couldn't follow why it was changed:
2b07e69

The problem that we are experiencing is that we are updating a query in the store with ReadWriteTransaction and in the previous versions the updated query captured the newly dependent keys, therefore when a change occured it was propagated to the result handler and now it's not.

What's the reason behind this change and how can we workaround this? Was there a problem with the previous implementation in 0.22.0 https://github.com/apollographql/apollo-ios/blob/0.22.0/Sources/Apollo/GraphQLQueryWatcher.swift#L66-L68

@designatednerd
Copy link
Contributor

Hi, the commit you referenced was part of #1156 which went out with 0.28.0, not 0.23.0. You can see a detailed explanation of the bug in #1155.

What version are you actually on right now?

@designatednerd designatednerd added caching question Issues that have a question which should be addressed labels Aug 20, 2020
@teodorpenkov
Copy link
Author

hey @designatednerd thanks for reaching out.

I'm using 0.28.0 but I think the problem that I experience is on every version after 0.23.0.

Our use case is pagination using the Apollo cache.
We are doing one main query followed by 0 or more fetch more queries (based on page info and scroll events) each fetch more query updates the main one with its nodes and page info. We've been using this method for a while and it worked pretty well, the reason for that is that the "main" query was updating its dependent keys (the nodes from the fetch more queries) and after the commits I mentioned it stopped working.

Mutation values gets to the store correctly but after didChangeKeys gets called this results in a watcher's result handler being called with the old values rather than the updated values from the mutation.

I will run through the commits and PR's that you mentioned and will try to figure out what the problem with the previous behavior was.

@designatednerd
Copy link
Contributor

So I know some stuff changed around 0.22.0 that was the motivation for these changes that went out with 0.28.0, but otherwise I'm not really seeing anything that changed. If you have any suggestions or a PR, I'm more than happy to try them out!

@teodorpenkov
Copy link
Author

I'd either suggest reverting to 0.22.0's implementation https://github.com/apollographql/apollo-ios/blob/0.22.0/Sources/Apollo/GraphQLQueryWatcher.swift#L66-L68

or adding self.dependentKeys = graphQLresult.dependentKeys before this line https://github.com/apollographql/apollo-ios/blob/main/Sources/Apollo/GraphQLQueryWatcher.swift#L79

If you don't see problem with that I'm happy to open a PR.

@teodorpenkov
Copy link
Author

teodorpenkov commented Aug 25, 2020

@designatednerd thanks for the links that you shared. I've checked them and I found the root cause of the problem.

Screen Shot 2020-08-25 at 17 05 17

I'm pretty sure that if we bring back those lines it wouldn't break anything.

@designatednerd
Copy link
Contributor

Hi! Sorry, I was on vacation last week.

I read your explanation here and I think I get it. I'll take a look at this today or tomorrow and see what I can figure out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caching question Issues that have a question which should be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants