Apollo-client: [Discussion] fetchMore is a bad match with cache redirects (type policies)

[darkbasic]

As outlined in a previous comment (https://github.com/apollographql/apollo-client/pull/6350#issuecomment-636176607) cache redirects can be annoying because when you use cache.modify you will have to remember to update the fields which feed the type policy instead of the ones you're actually working on.

Another thing worth mentioning in the docs is that fetchMore doesn't also play well with cache redirects: it will write a new query to the cache with the new elements appended. Probably you want to keep both queries (the source one used in type policies and the target one) in sync with the latest changes, so you'll have to remember to manually update the source one in the updateQuery method of fetchMore.

How to improve the current state of the things? In a previous comment (https://github.com/apollographql/apollo-client/pull/6289#issuecomment-631416262) I suggested that type policies seem half baked and that would be nice to be able to use them the other way around as well: instead of having to use the update method, whenever I insert a new item or remove an existing one I would love to be able to define a policy for that, which will be in charge to update the rest of the cache. That would resolve all of the previous issues and would also help to decouple the cache updating logic to the specific components (which shouldn't have knowledge of the rest of the application).

Unfortunately I don't know how to turn this into a GitHub Discussion.

[benjamn]

I'm sure what you're describing makes perfect sense to you, given your current understanding of the design space, but it would really help if you could put together a couple of concrete reproductions, so we can move past generalizations and get into the details.

[darkbasic]

@benjamn fine, here is the repro: https://github.com/darkbasic/apollo-gh6394
I've tried to think of an example which makes some sense, like a list of articles with comments. I've created it super fast so there may be some logic flaws, but I've put there some of the issues I'm facing in bigger real world applications.
Let me know if my previous message makes some sense now, otherwise I'll provide more context and/or explain it better.

[darkbasic]

@benjamn please refetch, I forgot to add the logic to update the source query in fetchMore.
As you can see things can easily go crazy when you deal with cache redirects. That's why I would love to centralize this logic, maybe extending type policies in order to also handle insertions of new elements/deletions of existing ones: that way if we detect that a new ref is being added to the comments query we could write some logic to also add it to the article(id) query without having the component to deal with it (the component is not supposed to know if it's using cache redirects).

[darkbasic]

Basically I want type policies to handle these things:

• Retrieve article(id) from articles
• Retrieve comments(articleId) from article(id)
• If a new comment gets added to comments(articleId) also add it to the article(id) comments field
• If a new comment gets deleted from comments(articleId) also delete it from the article(id) comments field

Right now type policies handle the first half of the list but not the second half and that creates a lot of confusion. If they handle redirects they should also handle insertions/deletions: the source and target queries must be kept in sync by some custom defined logic.

[benjamn]

Responding to this comment from the reproduction:

You could omit "first"/"last" from the key args, but then how are you supposed to know if you need to append or prepend the new comment? Because you can paginate both forward AND backward depending on which argument you specify between "first" or "last".

@darkbasic You absolutely should omit pagination control arguments from your keyArgs, and instead use a merge function together with your read function to maintain an internal list of objects that the read function can use to answer any options.args.{first,last} requests.

I would even suggest using keyArgs: false when you have both a read function and a merge function, so they can take full control over the interpretation of arguments, and the StoreObject key name will just be the original field name, without any serialized arguments appended to it.

This approach will make the read function much more effective for paginated lists, since it can interpret arbitrary options.args.{first,last} arguments, rather than relying on those exact first/last arguments having been used in the past. You don't really need cache.modify for this, but you will need a merge function to cooperate with your read function.

[darkbasic]

@benjamn I would really love to, but I'm still facing several shortcomings with the appoach described in https://www.apollographql.com/docs/react/v3.0-beta/caching/cache-field-behavior/#handling-pagination

The second example shows some kind of super-simplified one-way cursor base pagination, but:

1. The first thing I notice is that it doesn't store Connections or Edges, but just Nodes. This may be fine, but how is my infinite scrolling component supposed to know if we can fetchMore without a hasNextPage/hasPreviousPage boolean field?
2. The example works well because it only supports forwards pagination, but what if we mix both forward and backward pagination together? Let's say that we have a query the shows oldest articles and another one which shows newest ones: we start with the newest and when we go backwards it fetches more results as expected, until at some point the second query fetches the oldest ones. How is it supposed to know if we already fetched backward enough to reach the oldest articles or if we should fetch from the network instead?

Do you have any example of Relay-style pagination (forward and backward) with read/write functions?

[benjamn]

I'm going to work on some helper functions to generate field policies for pagination today, and I will make sure they cover these use cases.

As I mentioned over in https://github.com/apollographql/apollo-client/issues/5951#issuecomment-642313482, fetchMore is definitely broken right now, and fixing it will require some breaking changes, so I'm hoping to make the transition as easy as possible (hence the helpers).

[darkbasic]

@benjamn thanks! I personally don't care about breaking changes at this stage of AC3 development, for what concerns me take your freedom to make the API as pleasant as possible to work with.

Btw any news about exposing args to cache.modify? It's something I really need in order to be able to finally modify queries with filters.

[benjamn]

@darkbasic Have a look at #6465 for my take on Relay-style pagination via field policies.

While the relayStylePagination helper function technically could be written by anyone, I have to admit it was eye-opening to see how many edge cases have to be handled, and how important it was to write thorough tests. Thanks to the helper function, hopefully no one else will have to think about those details.

I still think it's a good idea to provide options.args to cache.modify, but I don't think it's going to make it into 3.0.0. Sorry about that. We have to draw a line somewhere, and options.args feels more like a worthwhile feature than a bug that should block the launch.

[benjamn]

@darkbasic I've been thinking more about options.args, and I believe I have a good workaround for you.

For any field that you plan to modify with cache.modify, if you need to examine the most recent arguments, you can store the arguments yourself using a read and merge function, and everything outside the cache will work just like it did before:

new InMemoryCache({
  typePolicies: {
    ParentType: {
      fields: {
        someField: {
          read(existing) {
            // Since the read function ignores existing.args and just returns existing.value,
            // the args are effectively hidden from normal code that reads from the cache.
            return existing?.value;
          },
          merge(existing, incoming, { args }) {
            return {
              // You could adjust/filter the args here, or combine them with existing.args,
              // or whatever makes sense for this field.
              args, 
              value: doTheMerge(existing?.value, incoming, args),
            };
          },
        },
      },
    },
  },
})

If this becomes a common pattern, you could wrap it up in a helper function:

new InMemoryCache({
  typePolicies: {
    ParentType: {
      fields: {
        someField: fieldWithArgs((existingValue, incomingValue, args) => ...),
      },
    },
  },
})

function fieldWithArgs<TValue>(
  doTheMerge: (
    existing: TValue,
    incoming: TValue,
    args: Record<string, any>,
  ) => TValue,
): FieldPolicy<{
  args: Record<string, any>;
  value: TValue;
}> {
  return {
    read(existing) {
      return existing?.value;
    },
    merge(existing, incoming, { args }) {
      return {
        args, 
        value: doTheMerge(existing?.value, incoming, args),
      };
    },
  };
}

Later, when you call cache.modify, the arguments you saved will be accessible in the modifier function:

cache.modify({
  id: cache.identify({ __typename: "ParentType", parentId }),
  fields: {
    someField({ args, value }) {
      // Examine args to help with processing value...
      return { args: newArgs, value: newValue };
    },
  },
})

I honestly think this workaround gives you more power to get things right than any automated options.args solution would. For example, you could maintain a historical list of all arguments objects received so far, and then cache.modify could look back in time, rather than relying on just the most recent args. I can't think of a good use case for that idea, but that's the kind of freedom that read and merge functions give you.

[tafelito]

@darkbasic Have a look at #6465 for my take on Relay-style pagination via field policies.

While the relayStylePagination helper function technically could be written by anyone, I have to admit it was eye-opening to see how many edge cases have to be handled, and how important it was to write thorough tests. Thanks to the helper function, hopefully no one else will have to think about those details.

@benjamn it's great to have something like this baked in the core. Although an offset/limit pagination is not as complex as the relay style, I think it'd be nice to also have it baked in, or at least some helpers. It took me time to come up with what I commented end up writing here https://github.com/apollographql/apollo-client/issues/5881#issuecomment-643058788
I'm not entirely sure tho if this is something that can be done from the AC side, since offset/limit pagination could be different from user to user compared to relay that always follow the same structure.
As you said, merge and read is a powerful tool, but with great power comes great responsibility 😁

[tafelito]

@benjamn I just had to do exactly that, I needed to access the args in order to update the cache. Returning them from the merge function works perfectly for that use case.

[tafelito]

by the way is the is the read function supposed to be called even when using fetchPolicy: 'network-only'?

[benjamn]

@tafelito Yes, network-only still writes its result to the cache, and reads it back from the cache before delivering it, so read functions are called. The no-cache fetch policy will completely ignore the cache.

[tafelito]

OK got it. And is there anyway to know from the read what fetchPolicy was used from the read function?

[tafelito]

@benjamn sorry to keep iterating over this but you said that using network-only writes to the cache and reads it back from the cache before delivering it. ~The first part seems to be true, every time the query is executed the cache is updated, but the read is only called once. I put a console log in the read function and only logs it one time. I inspect the cache and the values are updated there but I can't figure out why the read is not being called~
Sorry that's not accurate, the merge is also not being called after the first time, so the data in the cache is not updated

[tafelito]

One thing I noticed is that for the reason why the merge is not called, and I'm still trying to figure it out by debugging it, is that the data that comes from the fetch is somehow cached by AC somewhere.
When I looked at the network tab, this is the result I get

As you can see the totalCount is 3 and the nodes are 3 objects as well. But then, when I debug it, in this function

the result always has totalCount 2 and 2 items in the nodes array. And because the data is the same as the previous, then the merge is never called.

Is there anything else I can try to try to figure this out?

[jedwards1211]

I still haven't seen someone make a compelling case that storing last args would do more harm than help. Having last args wouldn't prevent you from having "enough power to get things right" by employing more complicated schemes. Y'all haven't really explained why you're actually averse to storing last args. Not wanting to add more API surface area?

If someone needs a bizarre use case like looking back through the history of args on a field, let them go to extra trouble to implement that, instead of going to extra trouble for all the common cases that would be solved by having access to the last args (which are inherently tied to the last query results).

To me the balance between ease of use and power user features is taking a nose dive in the power user direction.

[jedwards1211]

For me a more systematic solution to pagination, if the cache isn't going to store last args, would be to make my generic server-side pagination resolver echo the search/sort/pagination args back alongside the list in the query result. At least then I wouldn't have to specifically add this workaround to field policies anytime I create a new paginated field.