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

Investigation: fine-grained async control over caching behaviour #4693

Closed
TkDodo opened this issue Dec 23, 2022 · 7 comments
Closed

Investigation: fine-grained async control over caching behaviour #4693

TkDodo opened this issue Dec 23, 2022 · 7 comments
Labels
question Further information is requested v5
Milestone

Comments

@TkDodo
Copy link
Collaborator

TkDodo commented Dec 23, 2022

Related:

One approach would be to have another async function that runs before the queryFn (and maybe one that runs after it ?) that would skip calling the queryFn if something other than undefined was returned. Here, people could read from an async storage (and potentially write to it in the "after" hook). The query could also stay in fetchStatus: 'idle' because we haven't triggered the queryFn yet.

Combined with the possibility to customize the in-memory cache (see #4687), people would be free to keep the size of the in-memory cache small.

@TkDodo TkDodo added question Further information is requested v5 labels Dec 23, 2022
@TkDodo TkDodo added this to the v5 milestone Dec 23, 2022
@incepter
Copy link
Contributor

Here are the cache insights from my lib:

The CacheConfig accepts a:

  • a load function that can be sync or async, and returns a record of a CachedState shape, in the case or react-query, it would be just the cached state
  • an onCacheLoads callback that may be used to imperatively set the state from cached data.
  • a getDeadline function that receives the Succeeded state, and returns the state time.

The run of my producer (query for react-query) first calculates the hash of the run (the query key in your case), then checks if there is a non expired cache entry, then set's it as current state (if not already).

I hope these insights provide any help.

Im not linking my lib to avoid being targeted as an ad.

@TkDodo
Copy link
Collaborator Author

TkDodo commented Dec 26, 2022

One thing we could do is add an onQuery callback to the QueryCache itself. This would be similar to the onMutate callback of useMutation - it would run before the queryFn.

It could be async, and when something other than undefined is returned, we would use that as the result for this query instead of calling the queryFn.

The query could also stay in fetchStatus: 'idle' because we haven't triggered the queryFn yet.

Not sure this is possible or desired.

and maybe one that runs after it ?

This is what the onSuccess callback could be used for.

In combination with #4687 , we could so something like this:

const queryClient = new QueryClient({
  queryCache: new QueryCache({
    cache: createMyCustomCache(),
    onQuery: async (query) => {
      const data = await readFromAsyncStorage(query.queryKey)
      return data ?? undefined
    }
    onSuccess: (data) => {
      writeToAsyncStorage(query.queryKey, data)
    }
  })
})

@DamianOsipiuk
Copy link
Contributor

@TkDodo I think this topic connect to the one we have discussed previously.

Potentially we could introduce a persister argument to the query options.
If provided, persister could be run instead of a queryFn.
persister could have following implementation on a high-level (and be provided by @tanstack/local-storage-persister etc.):

function persister(context, queryFn, persisterOptions) {
  // 1. Check if data in `QueryCache` for given `queryKey` exists (means it's been restored already or fetched). If yes, then we skip the persistance layer logic and move to point 4
  // 2. Query the persistance layer for stored data by `context.queryKey`
  // 3. If stored data matched the `predicate` or `staleTime` from `persisterOptions` just return it and skip the `queryFn`
  // 4. If data don't match or does not exist, run `queryFn`
  // 5. store data returned by `queryFn` in the persistance layer (possible configuration in `persisterOptions`)
  // 6. return fetched data
}

Benefits of such implementation:

  • framework agnostic persistance plugins (just a js function), needs to be written once and serves all adapters
  • fine-grained control over which queries to persist
  • per-query persistance options
  • possibility to persist everything with defaultOptions
  • possibility to persist branch of queryKey with setQueryDefaults
  • removes initial QueryCache restoration delay and connected logic, cause query state would be restored lazily just-in-time when query needs the data
  • it does not matter if persistance layer can be accessed synchronously or asynchronously as it's run in a promise (no need for a separate sync-stoage-persister)

Drawbacks:

  • Query data will not be immediately available, Promise needs to be queued even for localStorage access
  • This means that app will be in a loading state for a brief second. But i think that this should be fine as users should be handling loading state anyways.

We could even start those new per-query persisters as a wrappers for queryFn as a POC without extending API layer.

@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 4, 2023

Promise needs to be queued even for localStorage access

Yes, we have to make this async, which is fine 👍

This means that app will be in a loading state for a brief second. But i think that this should be fine as users should be handling loading state anyways.

so when the persister runs and restores on page load, we don't subscribe the observer (even though it's already created). That makes sure your app renders, but it will be in pending state and fetchStatus: idle. Then, after restoring is done, we'll trigger the subscription, which might trigger a refetch - or not, if the data we now have is already fresh.

So I think this drawback would be fine - Whatever we do should not run if we already have fresh data in the cache. That's why I like the idea of a wrapper around the queryFn that could be exposed by the persister package, because it could not only take care of the read operation, but also the write back to the cache.

My other idea was similar to yours, except that I was leaning more towards an onQuery method (similar to onMutate on mutations). The idea was to run this function right before the queryFn, and if it returns a resolved Promise, we'd put that in the cache and don't run the queryFn. If the returned promise fails, we'll go on and run the queryFn. But this wouldn't handle the "writing back" part - I was thinking this would then need to happen in onSuccess of the QueryCache or so.

So I kinda like the wrapper idea. It would also keep the size of the core package the same because it would come from the persisters. However, I think it would be harder to set it globally unless you use a default queryFn...

@TkDodo
Copy link
Collaborator Author

TkDodo commented Feb 14, 2023

@DamianOsipiuk would you like to give the wrapper function a try? I'd imagine something like:

useQuery({
  queryKey: ['my', 'key'],
  queryFn: withPersist(options)((ctx) => fetchQuery())
})

that way, you could create your custom wrapper by moving the withPersist declaration somewhere central:

const myPersister = withPersist(options)

useQuery({
  queryKey: ['my', 'key'],
  queryFn: myPersister(fetchQuery)
})

@elraito
Copy link

elraito commented Feb 14, 2023

Im not really versed in developing a library as a newbie but im very invested in seeing a solution as I have currently a react native project that could greatly benefit from separate query caches. The queries can contain several megabytes of base64 strings and loading/storing them all at once is slow with my current implementation. My hacky solution has been so far to create separate queryclients and bringing the provider down the component tree but that solution is hard to maintain.

I like all proposals so far.

If i understood the proposal correctly my ideal solution would be to add persiter field to useQuery hook options object that would take in an object similar to persistclient configuration. The hook would first read persister value with get callback and set the result as initialData if successful. If not successful continue with regular fetch function. If stale time is configured and hasnt elapsed it would set the initialData as data and consider the query as success. If the stale time has lapsed or persister fails to provide info the queryFn would fire based on other rules in options object. Callbacks would then fire based on queryFn results. If the queryFn is a success then persister set function would be called and new result stored in persister. If invalidateQuery is called with matching key then persister del function would be called. This would fail the persister get step next time the query is used and query would force fetch anyway.

Am i correct that the proposal is at least in same spirit? Cause if yes then that would be an amazing addon to the library.

Option b I also like is if the persistclient get/set functions would optionally take in a serialized key so that each query could be seperatly handled and the developer has the responsibility by writing the correct get/set/del functions. Not exactly sure what the API would look like. Perhaps createPersister(separateKeys?: boolean) or something? Not sure.

In any case i wish you guys all the inspiration. Looking forward if the functionality can be implemented in any way so that the lambs are well and the wolves have eaten.

@DamianOsipiuk
Copy link
Contributor

@elraito Yeah, this is the rought idea.

@TkDodo Yeah I can prepare a POC. But probably next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested v5
Projects
None yet
Development

No branches or pull requests

4 participants