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

[Feature Request] Caching Directives: @apollo_ios_ignore_key, @apollo_ios_append #3004

Closed
Iron-Ham opened this issue May 10, 2023 · 6 comments
Labels
feature New addition or enhancement to existing solutions

Comments

@Iron-Ham
Copy link
Contributor

Iron-Ham commented May 10, 2023

Use case

There are several options for paginating a GraphQL API. A common implementation is the Relay style connections implementation -- which we make heavy use of at GitHub.

Take the following GraphQL stub:

query ViewerFollowers($first: Int!, $after: String) {
  viewer {
    followers(first: $first, after: $after) {
      pageInfo {
        hasNext
        endCursor
      }
      nodes {
        login
        avatarUrl
      }
    }
  }
}

This query works well enough if we're just launching them individually. Now, imagine that we'd like to watch the result. Using the ViewerFollowers query for the first page will work just fine. However, should we want to update the query for subsequent pages, we will no longer be watching the correct query; the original query is stored with its variables in place, and subsequent queries are stored with their variables.

In 0.x versions of Apollo, this was handled by fetching subsequent page, replacing the pageInfo object within the cache, and appending the new results to the nodes (some techniques were discussed in #1393). However, due to the immutable nature of Apollo 1.0 queries, this presents a difficult problem to solve:

We can no longer append the values to the original query, as the query is immutable, and defining a LocalCacheMutation for this would mean that every paginated network call would have to define its own LocalCacheMutation; we no longer have the ability to generically paginate requests.

Describe the solution you'd like

We could add new directives.

@apollo_ios_ignore_key

This directive could be used to ignore a variable within the list of variables. For example, take the previous query. The number of items pulled as part of that request and the endCursor aren't relevant to cache-key storage. New requests with different keys should overwrite (or append) extant data.

In effect, this would mean that a GraphQLOperation's Variables would be filtered by presence of this key for the purpose of calculating a rootCacheReference.

@apollo_ios_append

Over-writing data is the default, but for lists of items -- as is common in pagination -- we may instead want to append the new results to the old results. This key would be used for that specific purpose, such that the nodes we get in the second, third, or fourth request with a different endCursor are appended to the original set of nodes.

In effect, this would mean that some fields of a query or fragment may be generated with mutability (or new caching logic).

Alternatives considered

  1. Something more similar to Apollo React's merging and field policies would also be a viable alternative.
  2. The creation of some sort of pagination interceptor. This interceptor can be appended into the existing set as an argument for a specific fetch operation. Something like this could be a rudimentary implementation that can be put in place of the CacheWriteInterceptor by consumers. Crucially, this would require a change to GraphQLResponse such that the variables property is (1) mutable and (2) public. However, this rudimentary implementation doesn't actually solve the problem of appending data to the cache; it merely replaces the existing data by doing what @apollo_ios_ignore_key could accomplish
  3. Queries/fragments marked these ways generate setters as well as getters, and the onus of updating the cache is tossed back to the end user.
  4. A new object, GraphQLPaginatedQueryWatcher that can abstract over n watchers and provide an output stream of data. The underlying data source would have to be a sort of an OrderedSet, where the most recent insertion of data is the canonical one. I am going to investigate implementing this now and will update in real time.

Somewhat related to #2984

@Iron-Ham Iron-Ham added the feature New addition or enhancement to existing solutions label May 10, 2023
@Iron-Ham Iron-Ham changed the title Caching Directives: @apollo_ios_ignore_key, @apollo_ios_append [Feature Request] Caching Directives: @apollo_ios_ignore_key, @apollo_ios_append May 10, 2023
@AnthonyMDev
Copy link
Contributor

Sorry I didn't get a chance to look at this earlier!

Pagination has been a long standing pain point that we haven't addressed well, and rather than injecting new data into existing models I'd prefer us to actually expose APIs that handle pagination directly and in a type-safe way. I'm excited to take a look at your Paginated Query Watchers PR this afternoon!

But it's worth noting, even though the property accessors on the models are immutable, you could still access the underlying DataDict and mutate the underlying data. It's just not type-safe or officially supported.

Also, you can add @apollo_ios_local_cache_mutation to a named fragment and make that specific fragment mutable. I'm not sure if that solves your whole problem here, but it might make this easier.

There has also been significant community support for making the existing operation models mutable again. It's not what I think is the "right" approach, but the amount of people asking for it is causing us to consider it.

@Iron-Ham
Copy link
Contributor Author

No worries re: response time!

Pagination has been a long standing pain point that we haven't addressed well, and rather than injecting new data into existing models I'd prefer us to actually expose APIs that handle pagination directly and in a type-safe way. I'm excited to take a look at your Paginated Query Watchers PR this afternoon! But it's worth noting, even though the property accessors on the models are immutable, you could still access the underlying DataDict and mutate the underlying data. It's just not type-safe or officially supported.

I didn't feel comfortable recommending to consumers of an API that they dig about the underlying data, and it requires a bit too much knowledge of the construction of any specific response model to be feasible for wide-scale use. I am, ultimately, thinking about this fanning out across several thousand queries + fragments within a single codebase and it just doesn't scale well.

But to your point: I fully agree that I'd like it to be type safe, that I'd like us to handle pagination directly, and that I'd like for it to jive with immutability. The community may be reaching to undo the move towards immutable models -- which is a logical reaction to being placed into a new landscape without an explicit viable alternative being provided. Truthfully, that was my first gut instinct too -- and I went about modifying code generation, modifying interceptor protocols to quietly mutate data, and so on. Ultimately, where I landed was to roll forward instead of rollback. Whether the PR I have up is the right approach or not, I don't know. Happy to workshop it -- but I'd rather look for the right solution with the ideals and principals that guided the move towards immutability rather than claw back an admittedly janky solution from the past.

@teodorpenkov
Copy link

In the end what's the recommended way to paginate? Merging and field policies from React seems like the best way. The current state of apollo-ios prevents you to do anything similar to the react approach.
Mb listening to the community and brining back the mutability of the generated models is not such a bad idea?

@Iron-Ham
Copy link
Contributor Author

Iron-Ham commented Oct 4, 2023

@teodorpenkov I'm exploring pagination further here: https://github.com/apollographql/apollo-ios-pagination

Note that the current implementation has some bugs and isn't quite ready for release yet.

@AnthonyMDev
Copy link
Contributor

@Iron-Ham Is this feature request still relevant with the pagination library, or should we close this one?

@Iron-Ham
Copy link
Contributor Author

I think it can be safely closed!

@Iron-Ham Iron-Ham closed this as not planned Won't fix, can't repro, duplicate, stale Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New addition or enhancement to existing solutions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants