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 GraphQLQueryWatcher Query Public #1037

Merged
merged 1 commit into from
Feb 25, 2020

Conversation

giantramen
Copy link

Allow the using app to access the query on
the watcher.

It makes our life a lot easier to be able to access the query on watchers so we can update the cache for things we are watching if we know they changed.

Allow the using app to access the query on
the watcher.
@designatednerd
Copy link
Contributor

Can you explain how this helps in a little more detail? I'm trying to keep stuff that isn't already public non-public unless there's a good reason not to. It sounds like this might be one, but why not (for example) have a variable name that makes it clearer what query this is tied to rather than exposing this property publicly?

@giantramen
Copy link
Author

giantramen commented Feb 24, 2020

I think any solution is fine that allows someone to access the query after they have created the watcher.

My exact use case is we have a paginated list of images and a watcher for each query. The query has a list of images and a total number of images. When a user deletes a photo we need a way to update those totals on each query in cache, which AFAIK can only accessed by the query.
Because we are already keeping the watchers, we iterate over those and use the queries to update the cache.

for watcher in paginatedViewModel.watches {
            apollo.store.withinReadWriteTransaction({ transaction in
                do {
                    try transaction.update(query: watcher.query) { (data: inout T.Data) in
                        if var poiMedia = data as? PoiMediaListQuery.Data {
                            poiMedia.poiMediaList?.totalCount -= 1
                            // This was just T.data it is safe to cast it back
                            // swiftlint:disable:next force_cast
                            data = poiMedia as! T.Data
                        }
                    }
                } catch let error {
                    AppLogger.debug("Failed to update paginated image cache: \(error)")
                }
            })
        }

We theoretically could keep the queries around as well, but it seems to make sense to me that because the user has to create the watcher with the query that exposing it isn't a big deal.

@designatednerd
Copy link
Contributor

OK and it seems like you can't just swap the poiMedia.poiMediaList?.totalCount for just getting count since that's probably going to contain some unloaded data and totalCount could be way way more than count.

I'll probably go ahead and approve this one since there's nothing on Query that can be modified so the potential for damage isn't totally awful, but this sure seems like a super-roundabout way of updating this information.

I would recommend talking to your backend team about having some kind of mutation that would alter the poiMediaList and hand info about it back, so that you could have that handle updating the totalCount rather than having to do it manually.

@designatednerd designatednerd added this to the 0.24.0 milestone Feb 25, 2020
@designatednerd designatednerd merged commit 4c82e5a into apollographql:master Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants