Pagination: Pager does not provide updates during a refresh

[pixelmatrix]

Summary

It looks like AsyncGraphQLQueryPager does not provide updates to previously fetched objects that have changed while it's being refreshed.

I'm not sure if this is more of a bug or a feature request, but it's causing problems for our use case.

Expected behavior:
Changes to the data originally provided from the pager is updated while an in-flight refresh request is active.

Actual behavior:
The data provided from the pager does not update until the refresh is complete.

Version

1.10.0

Steps to reproduce the behavior

We're using the pager to fetch all pages for a particular query. Later, when we need to refresh the data we call loadAll(fetchFromInitialPage: true). This particular query is a bit slow, so it can take a couple of seconds before it completes. In the meantime, we allow the user to interact with the items in the pager, which results in mutations.

It seems while the query is being refreshed, the pager does not clear the results via the subscribe callback, but it also does not emit any updates for local cache mutations or updates received from subscriptions until the pager completes its work.

Repro steps:

1. Setup an AsyncGraphQLQueryPager with a subscribe handler.
2. Load all the data using loadAll(fetchFromInitialPage: true).
3. Start a refresh using loadAll(fetchFromInitialPage: true).
4. While the refresh is pending, make a local cache mutation. You should observe that the subscribe handler is not called.
5. When the refresh has completed, you should observe that the subscribe handler is called.

Logs

No response

Anything else?

No response

[BobaFetters]

@pixelmatrix Thanks for the detailed description and reproduction steps, as you said I'm not sure if this is a bug or a feature request. I will check with the team so we can see what the path forward here is.

[Iron-Ham]

@pixelmatrix Ah, I see exactly what you mean.

It's a difficult problem to solve for: calling loadAll(fetchFromInitialPage: true) will reset the pager. When a reset is triggered, it effectively unsubscribes from all underlying GraphQLQueryWatchers and begins the process of re-creating watchers from scratch. Vacating old watchers is important – as query arguments can change between fresh loads (if you have data that actively changes, like a notification feed, the end cursors of each page aren't guaranteed to be the same after a refresh).

We can certainly allow cache updates in the subscribe logic but given that the watchers were unsubscribed from during this process, there may not be a watcher to trigger that update in the first place. I have a pull request proposing to remove this restriction from the subscribe logic, but do note that there will be a behavioral change: You will receive an update each time you get a page, as opposed to only when we have the full set of data.

Until this load is complete, the pager's internal data state is incomplete: It has deleted its previous state and is constructing the new state.

I imagine that any cache change at this time is an optimistic mutation that should be reflected in the new data? In that case, there would be a few ways to solve for this:

• You could fire off the optimistic update and allow the mutation to respond with the updated data, which should trigger a cache mutation without needing to reload all data.
• You could keep your existing logic in full, but otherwise just make an in-memory change of your data, so that your UI can update while you're re-fetching in the background.

Would either of these work for your use-case?

---

cc: @AnthonyMDev perhaps an examination of the subscribe method and the assumptions we make are warranted. The current logic makes some assumptions around how users want to be updated. A solution to this problem – and ones like it – is to fully remove those assumptions, such that we let all updates through. The current assumption is that if a user calls loadAll, they don't really want to be notified until we can furnish all the data they requested. However, I could see a situation where a user would want the pages as they come in: it may be noisy for UI updates, but not all use-cases are UI. To accommodate for that change, we would then have to do a few things:

1. Make the isLoadingAll property available on the public-facing AsyncGraphQLQueryPager and GraphQLQueryPager. See: [ApolloPagination] Make isLoadingAll publicly accessible, remove special logic apollo-ios-dev#371
2. Maybe we should consider removing the convenience subscribe methods that we have in place in GraphQLQueryPager and AsyncGraphQLQueryPager. We make heavy use of these methods in the GitHub app, since they conveniently dispatch to the main thread (useful for UI updates!) but I wonder if they're doing more harm than good? Removing them would push management of an AnyCancellable to the user, instead of internally managing them within the pagers (removing risk of future potential threading errors in the library, since we're frequently interfacing between Combine/async-await/actors). See: [ApolloPagination] Remove convenience subscribe methods from GraphQLQueryPager and AsyncGraphQLQueryPager apollo-ios-dev#370

These are breaking changes but would push users to use the Combine Publisher APIs for interacting with the GraphQLQueryPager. For example:

pager.sink { [weak self] result in
  guard let self else { return }

  if self.pager.isLoadingAll { /* Custom logic */ }
  // Whatever the user wants to do 
}.store(in: &cancellables)

[pixelmatrix]

Yeah, this does seem like a difficult problem. As you mentioned, the old state is deleted in the pager, while the new state is still pending. However, the UI / consumer of the pager is still using on the old state, so it's in this limbo.

In my specific use case, doing a full refresh typically does not change the data much, but we need to occasionally refresh still to ensure things are up to date. It's transparent to the user, since refreshing happens automatically. This makes it feel like a bug when data on screen does not immediately respond to mutations or subscriptions.

To your note about allowing progressive updates while loading all – that would actually be preferred in my case. Loading all the pages can take several seconds, and the first page generally includes enough data to fill the screen. It could be nice to see updates sooner.

You could fire off the optimistic update and allow the mutation to respond with the updated data, which should trigger a cache mutation without needing to reload all data.

The problem with this is that we trigger the refresh automatically when the screen appears. While the refresh is ongoing, the user performs a mutation, which results in an update to the cache, but that update does not come through the pager until the full refresh is complete. The goal of the refresh is to sync with other clients, rather than to update the data as a result of the mutation.

You could keep your existing logic in full, but otherwise just make an in-memory change of your data, so that your UI can update while you're re-fetching in the background.

Do you mean just updating the UI's copy of the data with the change based on the user's action? There are a couple of hurdles there. One problem is that the generated models are not mutable, so it either requires some hacks, or an abstraction to even change the data. The other challenge is that the component that owns the pager may not be aware that something has changed directly. Another view might have made the update, so we rely on the watchers to signal when something has changed.

The workaround I have implemented for now is to observe the ApolloStore directly and fetch from the cache again when a change is detected. That solves the issue for me, but it would be convenient if the pager could do it automatically, since it's a bit tedious to detect the changes and refetch.

[AnthonyMDev]

@Iron-Ham I think I'm in favor of removing the subscribe methods. It would simplify a lot of the implementation, and I'm usually in favor of having one opinionated way of doing things rather than giving too many options (and then have to maintain support for all of them).

I'm wondering if there is another solution for this though. While waiting for all of the pages to reload, two types of changes to the data could occur.

1. Changes to the contents of individual items in the paginated list
2. Changes to the order of items in the list (inserts, removes, moves, page cursor changing, etc.)

While reloading all pages, changes to the order of the list (2) are going to cause issues, which is why we are vacating the watchers during that time. But what if we continued to watch all of the items for pages already queried for changes during a refresh (1), and then return the entire new list only after the entire refresh is done (2)? We could then propagate updates from the cache to the existing list while waiting for the reload to occur.

This would probably require us to go the route of an internal ApolloStoreSubscriber and keeping track of the list of actual items and their cache keys, rather than just using a GraphQLQueryWatcher for each page's query. But that's something that we have already been talking about exploring for other reasons anyways.

One limitation here is that this would require the entities in your list to have their own unique cache keys configured, but that limitation could be well documented (and even validated at run time if need be).

[Iron-Ham]

I'm glad that removing the subscribe methods seems like the right path forward.

In general, inserts/removals can happen even when we're paging manually and can cause issues that aren't well handled by Apollo in general now; the problem is no better or worse here than it would be otherwise. A consumer of Apollo can handle this, but it's not easy unless they're using custom models. For consumers that primarily rely on the generated models, this introduces an issue – and one that we could solve by introducing custom directives to be added as decorators to the user's query.

I am curious to dive a bit deeper into the ApolloStoreSubscriber approach. My gut feeling is that I'm not sure it would work well for schemas that make heavy use of connection types (like GitHub's). Connection types don't have any identifier on them, and so wouldn't be able to update with any reorder/insertion/removal unless the calling query updates them (or a local cache mutation keyed with the query's variables mutates them).

[Iron-Ham]

Related to this – and part of why I think loadAll returning on each fetched page is a net positive – we could add a loadAll(until predicate: @autoclosure (Output) -> Bool) function that lets users load all until some condition is met – either with the response data or external to Apollo operations.