Added refetch in case cached data has gone stale.

[lucasconstantino]

This is a proposal and has tests failing

Discussing about cache invalidation I've rised a questioning on why there happens no refetch on ObservableQueries once data has gone stale.

This pull-requests presents with a small update to trigger a refetch when missing fields from cache are found. I will still provide stale data, but will provide new data once it is fetched.

There is one test failing, but I didn't want to go on with this without bringing the proposal forward to the Apollo team.

[stubailo]

We intentionally didn't include automatic refetching on stale data because it's likely to cause an infinite loop, and often the right solution is to change your app code. Stale data often comes up when you have two queries on the same page asking for conflicting data, and if they are querying rapidly changing lists you will sometimes end up with a ping-pong list refresh.

I could be in favor if this was an opt-out option, or perhaps it had some infinite loop detection like only refreshing 2 times before throwing an error or something.

[lucasconstantino]

@stubailo indeed I have seen this infinite loop possibility. To provide you with some context, I've recently publish apollo-cache-invalidation, a library to facilitate cache invalidation of fields. Thing is, the only way I found to make that idea work was purging specific data from the DataProxy directly. This eventually ends up with current ObservableQueries hitting stale data.

[stubailo]

Yeah I looked at that, it looks really cool actually!

So do you think there is a way to limit refreshing to only once somehow, and still keep the invalidation working?

[lucasconstantino]

There are easy and hard ways, I guess. Best way would probably be to allow for the imperative store API to actually hold information on recently invalidated fields, and erase that information once refetches are dispatched. But maybe I could write up an opt-in option. Something like shouldRefetchStale to be provided on the apollo client configuration object. It would then be called everytime prior to the refetching, giving it some context and allowing the user to decide whether or not to refetch. This way I could add this logic to the apollo-cache-invalidation module, by basically temporarily saving information on recently invalidated fields.

[stubailo]

Ah, so basically, when you invalidate you set a flag, something like "justInvalidated" and then when the query refetches, it sets that back to false.

Hmm, what would the shouldRefetchStale function take as arguments?

[helfer]

@lucasconstantino I think infinite loops are pretty unlikely here, unless the server state keeps changing very rapidly. I would be okay with having something like this be an optional argument to the query, maybe called refetchIfStale or something like that.

[lucasconstantino]

Ok, here is a new code proposal: I've added an option called shouldRefetchIfStale, which is a callback to be called when stale data is encountered. The callback defaults to a returning false callback, which should keep things as they are and introduce no breaking change, making it opt-in. The callback will be provided with some context that should make it easy for the user to decide if the stale data should be refetched or not.

This opens the possibility for more grained control over stale data refetching, while removing the need to inform each and every query that it should behave this way, thus facilitating adoption.

For one to decide to simply turn-on the functionality on all queries, it must simply provide a callback returning always true.

Not that elegant, but that's what I've reached so far. Any thoughts?

P.s.: @helfer infinite loops, as you said, are really unlikely to happen. Funny thing is that it('should return stale data when we orphan a real-id node in the store with a real-id node') test is doing exactly something that would make it occur :)

Edit: I could make shouldRefetchIfStale option receive both a callback or a boolean... I guess that would be fine, but I'm not always comfortable with an argument receiving multiple types.

[helfer]

@lucasconstantino I think having it as a function is a good first step. Just to make sure we're 100% on the same page, how would you use said function to avoid a loop? I.e. can you provide a complete example here?

I'm also curious to know how that test triggers an infinite loop. Maybe there's something I haven't considered.

[lucasconstantino]

@helfer just to point it out, I've only today read issue #821, and thus I now understand the problem I'm dealing with here was thoroughly discussed previously, and for a long time. Automatically refetching stale data was something even proposed as a possible improvement, so I see this is no new matter for you.

I've pictured only one situation where infinite loops might happen when refetching stale data: when more than one ObservableQuery are listening to a query with a given field returning a given type which resolves the resource id to a new value every time it is called, and with different fields returned. Both would return different id's, meaning the second one's result would invalidate the first one's data, making the first perform a refetch, which would eventually invalidate the second's data, starting the loop again. This is only an assumption, though, and I haven't tested if this is really an occurring scenario.

The mentioned test is doing exactly that, and I've tried running all tests with the refetch turn on by default; that test is the only one that fails, even though I did not dig further to see if an infinite loop was what cause the failure.

---

shouldRefetchIfStale could be used to avoid infinite loops in any customized manner, such as storing recently refetched query ids and preventing them to be refetch in a given amount of time.

const refetches = {};

const shouldRefetchIfStale = ({ queryId }) => {
  const now = new Date()

  // Find's out if any refetch for the current queryId happened less than 10 seconds ago.
  if (refetches[queryId] && refetches[queryId].some(time => time > now - 10000)) {
    return false
  }

  refetches[queryId] = refetches[queryId] || []
  refetches[queryId].push(now)
  
  return true
}

In my situation, using apollo-cache-invalidation, I would probably add a helper to only enable refetching of recently invalidated fields, if someone has the need.

[lucasconstantino]

Any more thoughts one this matter? I can work on another approach if you guys find this one not suitable.

[helfer]

Hey @lucasconstantino, sorry I didn't have time to review this PR! Can you add a test case or post an example of how to use it in this thread? I think it's a useful functionality to add, but I want to make sure that it's really usable for people and that we have reasonable defaults.

❤️

[tnrich]

@lucasconstantino any movement on this? I would really like to have a way to invalidate parts of the apollo store and your solution seems like a reasonable way to do it :)

[lucasconstantino]

Sorry folks, had no time lately for this. This is indeed in my priority list, so I think I'll have looked into it by the end of the week.

[jbaxleyiii]

@lucasconstantino how can I help out on this? This functionality is a great one for the new effort around Apollos network layer.

cc @evanshauser for a use-case for apollo-link. We may even be able to just add it there instead of here?

[lucasconstantino]

@jbaxleyiii I'm really trying to put my hands on code on this one. Last week I rebased this against the current master, but I'm having some unrelated tests failure (even on the master branch). Have you looked at the code? That would be my first code contribution to the project, and even though @helfer said to go forward with it I would like some feedback on if you guys think this is the best approach - and if you see any TypeScript issues with the added code, for I'm not really used to typed code.

[lucasconstantino]

@jbaxleyiii @helfer I've just added some tests. Would anyone be able to do a code review on that?

[jbaxleyiii]

@lucasconstantino I can! I'll work to wrap it into the 1.9 release as we are getting ready to ship 1.8

[jbaxleyiii]

@lucasconstantino this change looks great! I'm going to sit on it for just a little bit longer as I believe we are really close to shipping Apollo-link. If we are able to hit that target next week, I will move this code into a link as it is such a perfect use case for it!

It is 💯 a need, either way in a short time it will be possible to evict some cache thanks to your help!

[apollo-cla]

	Warnings
⚠️	There are library changes, but not tests. That's OK as long as you're refactoring existing code

	Messages
📖	Please add your name and email to the AUTHORS file (optional)
📖	If this was a change that affects the external API, please update the docs and post a link to the PR in the discussion

Generated by 🚫 dangerJS

[jbaxleyiii]

hmm, danger seems to be broken on test detection here. I'll take a look

[jbaxleyiii]

for some reason it pulled from an older version of danger? It should actually be okay 👍

[stale]

This issue has been automatically marked as stale becuase it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Apollo Client!

[jbaxleyiii]

@lucasconstantino I do think that is should end up being moved to a link! I will chat with @evanshauser and we will get it all moved over! Thank you for the hard work! Sorry we weren't able to merge it in as initially expected.

[lucasconstantino]

@jbaxleyiii on which project do you think this functionality will be moved to? I could have a look there and maybe port the code on the free time ;)

[riccoski]

Did this get implemented?

[riccoski]

At the moment im using a custom query function to alter the fetchPolicy based on a TTL in order to force network-only request if it has expired.

https://gist.github.com/riccoski/224bfaa911fb8854bb19e0a609748e34

Unfortunately you have to manually create you CacheRef IDs as it's a lot of work trying to mimic how the client does it