This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d1d39b3:

Codecov Report

❗ No coverage uploaded for pull request base (v5@12659f5). Click here to learn what that means.
Patch has no changes to coverable lines.

@@          Coverage Diff          @@
##             v5    #4726   +/-   ##
=====================================
  Coverage      ?   90.73%           
=====================================
  Files         ?       89           
  Lines         ?     3487           
  Branches      ?      897           
=====================================
  Hits          ?     3164           
  Misses        ?      301           
  Partials      ?       22           

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Hello,

my understanding of the feature is to allow users to provide a cache with different strategies. My hope would be to provide a LRU cache like this one :

class LRUCache<K, V> {
  private cache: Map<K, V>;
  private order: K[];
  private capacity: number;

  constructor(capacity: number) {
    this.capacity = capacity;
    this.cache = new Map<K, V>();
    this.order = [];
  }

  public get(key: K): V | undefined {
    if (!this.cache.has(key)) {
      return undefined;
    }
    this.updateOrder(key);
    return this.cache.get(key);
  }

  public set(key: K, value: V): void {
    if (this.cache.has(key)) {
      this.updateOrder(key);
    } else {
      if (this.order.length === this.capacity) {
        this.evict();
      }
      this.order.push(key);
    }
    this.cache.set(key, value);
  }

  public clear(): void {
    this.cache.clear();
    this.order = [];
  }

  public entries(): IterableIterator<[K, V]> {
    return this.cache.entries();
  }

  public values(): IterableIterator<V> {
    return this.cache.values();
  }

  public forEach(callbackfn: (value: V, key: K, map: Map<K, V>) => void, thisArg?: any): void {
    this.cache.forEach(callbackfn, thisArg);
  }

  public has(key: K): boolean {
    return this.cache.has(key);
  }

  private updateOrder(key: K): void {
    const index = this.order.indexOf(key);
    this.order.splice(index, 1);
    this.order.push(key);
  }

  private evict(): void {
    const keyToEvict = this.order.shift();
    if (keyToEvict !== undefined) {
      this.cache.delete(keyToEvict);
    }
  }
}

So to allow me to provide this cache, i expect to be able to provide something else than a Map . would it be possible to change the QueryCache parameters to use a generic interface instead of Map ?

interface Cache<K,V> {
  has(key: K) => boolean;
  get(key: K) => V | undefined;
  set(key: K, value: V) => Cache<K,V>;
  delete(key: K) => boolean;
  clear() => void;
  entries() => IterableIterator<[K, V]>;
  values() => IterableIterator<V>;
  forEach(callbackfn: (value: V, key: K, map: Map<K, V>) => void, thisArg?: any): void;
}

interface QueryCacheConfig {
  createCache?: () => Cache<string, Query>
  onError?: (error: unknown, query: Query<unknown, unknown, unknown>) => void
  onSuccess?: (data: unknown, query: Query<unknown, unknown, unknown>) => void
}

class QueryCache extends Subscribable<QueryCacheListener> {
  config: QueryCacheConfig
  private queries: Cache<string, Query>
  
  ...
}

@ecyrbe have a look at my last commit:

• a358cb5

I've made the interface more generic. One problem with an LRU cache is that if you set the size to 5, and you have 6 queries on screen, it will remove an active query. With the next re-render, this query will start to go into a hard loading state and fetch, and once data comes back, it will want to write to the cache, at which point you'll remove another active query. This will probably go infinite.

Have a look at the test in the commit. I'm passing the queryCache instance into createStore, which will allow us to filter the whole cache for inactive queries and only remove those. So the max size is more like a "suggestion" (remove everything greater than this if possible), and active queries will always remain in the cache.

The weird thing is that the invocation is:

this.queries = config.experimental_createStore?.(this) ?? new Map<string, Query>()

so we pass this into createStore even though this.queries doesn't yet exist, and if we were to use the param in the constructor, it could die with a runtime error. I'm not sure how I feel about that hack 😅 . Thoughts?

Oh i see the issue now. So we can only apply cache mutations on inactive queries. So if we want to do LRU, the LRU cache would need to be able to :

• have access to Query active/inactive status
• notify that an inactive query was prematurily removed

I saw the experimental_createStore impl and example. I'm onboard with you, that it's kind of dangerous to do this kind of hack. It's like a chicken and egg issue.

Maybe the query store is the bad abstraction for this feature ? we want to delegate the cache handling strategy but still maintain control of how we handle active/inactive/notifications of the cache.

Here are other options :

• Have a QueryCacheBase and allow users to provide their own QueryCache impl that must use the QueryCacheBase. i'm not found of inheritance, so i would try other solutions before trying this one.
• Have the QueryStore set method have QueryCache passed as an optional last parameter. This way the store is not owner the Cache, i'm not found of this solution either since we still have a cyclic dependency

export interface QueryStore {
  has: (queryKey: string) => boolean
  set: (queryKey: string, query: Query, cache: QueryCache) => void
  get: (queryKey: string) => Query | undefined
  delete: (queryKey: string) => void
  values: () => IterableIterator<Query>
}

• make the QueryStore have callback you can subscribe to. This way the QueryCache will subscribe to it and handle the notifications for the QueryStore :

export interface QueryStore {
  has: (queryKey: string) => boolean
  set: (queryKey: string, query: Query) => void
  get: (queryKey: string) => Query | undefined
  delete: (queryKey: string) => void
  values: () => IterableIterator<Query>
  on(event: string, callback: ({ event: string, key: string, query: Query }) => void );
}

i like this last solution better since it breaks cyclic dependency. what do you think ?

here is an example implementation with LRU cache with events :

import { EventEmitter } from "events";

class LRUCache<K, V extends Query> extends EventEmitter {
  private cache: Map<K, V>;
  private order: K[];
  private capacity: number;

  constructor(capacity: number) {
    super();
    this.capacity = capacity;
    this.cache = new Map<K, V>();
    this.order = [];
  }

  public get(key: K): V | undefined {
    if (!this.cache.has(key)) {
      return undefined;
    }
    this.updateOrder(key);
    return this.cache.get(key);
  }

  public set(key: K, value: V): void {
    if (this.cache.has(key)) {
      this.updateOrder(key);
    } else {
      if (this.order.length === this.capacity) {
        this.evict();
      }
      this.order.push(key);
    }
    this.cache.set(key, value);
    this.emit("change", key, value);
  }

  public clear(): void {
    this.cache.clear();
    this.order = [];
    this.emit("clear");
  }
  
  public delete(key: K): boolean {
   const value = this.cache.get(key);
    if (value) {
      this.cache.delete(key);
      const index = this.order.indexOf(key);
      this.order.splice(index, 1);
      this.emit("delete", value);
    }
    return Boolean(value);
  }

  public entries(): IterableIterator<[K, V]> {
    return this.cache.entries();
  }

  public values(): IterableIterator<V> {
    return this.cache.values();
  }

  public forEach(callbackfn: (value: V, key: K, map: Map<K, V>) => void, thisArg?: any): void {
    this.cache.forEach(callbackfn, thisArg);
  }

  public has(key: K): boolean {
    return this.cache.has(key);
  }

  private updateOrder(key: K): void {
    const index = this.order.indexOf(key);
    this.order.splice(index, 1);
    this.order.push(key);
  }

  private evict(): void {
    for (let i = this.order.length - 1; i >= 0; i--) {
      const key = this.order[i];
      const value = this.cache.get(key);
      if (value !== undefined && !value.isActive()) {
        this.order.splice(i, 1);
        this.cache.delete(key);
        this.emit("evict", value);
        break;
      }
    }
  }
}

then on QueryCache :

export class QueryCache extends Subscribable<QueryCacheListener> {
  private queries: Map<string, Query>
  private queries: QueryStore

  constructor(public config: QueryCacheConfig = {}) {
    super()
    this.queries = config.experimental_createStore?() ?? defaultQueryStore()
    this.queries.on('delete', (query) =>  this.notify({ type: 'removed', query })
    this.queries.on('evict', (query) => {
      query.destroy();
      this.notify({ type: 'removed', query }
    })
  }
   remove(query: Query<any, any, any, any>): void {
    const queryInMap = this.queries.get(query.queryHash)

    if (queryInMap) {
      query.destroy()

      if (queryInMap === query) {
        this.queries.delete(query.queryHash) // on delete callback called
      }
    }
  }

evict means a delete of an inactive key without QueryCache requesting it.
delete event means a delete on a key handled by the QueryCache.

ps: all this was generated with the help of chatGPT, so it might have bugs.

Have the QueryStore set method have QueryCache passed as an optional last parameter. This way the store is not owner the Cache, i'm not found of this solution either since we still have a cyclic dependency

where would there still be a cyclic dependency in this example ? You won't be able to access cache.queries (which would be the store) because that's private...

I honestly like this solution best. It would still be compatible with a standard Map interface, and you will likely only need the whole cache in the setter function, so that you can interact with / remove other queries.

make the QueryStore have callback you can subscribe to.

We already have that on the whole QueryCache: queryCache.subscribe(event => ...). The possible events are:

If such an event would be enough, we wouldn't even need the custom store 😅 . Maybe it is ... ? You could listen to the event when a query is added, find all other inactive queries and remove them ...

I'm really starting to think it's two different issues:

• we can expose the store and createStore functions just so that users have some control over what structure to use
• we can still to side-effects, like manipulating what is stored how (size limit) via the already existing events on the QueryCache.

Also, you could still remove queries from the cache by finding out if they are active or not even without having access to the QueryCache - the store contains Query instances, so you have everything you'd need. It would just be a bit more effort without the QueryCache ...

@ecyrbe what do you think about this approach?

• abd1703

basically, we can have access to the cache in the setter via the Query that gets passed to it anyways 🤷

Hello @TkDodo ,
I'm maybe too of a perfectionist. when i see that the store needs to access parent methods, i see a circular dependency. Because :

• QueryCache calls QueryStore methods explicitly
• QueryStore calls QueryStore methods explicitly.

The issue i have is the "explicitly". I like when things are modular. Here if tomorrow you change the behaviour of QueryCache, you'll also need to change the QueryStore.
Ideally you'll want to not have to touch QueryStore if you change QueryCache.
This way you can guaranty that you'll not break userland. With the benefit of less maintainance and opened bugs.

@ecyrbe yeah I think we do have circular things in there already, because a Query lives in the QueryCache, but the Query also has access to the QueryCache itself.

In my last commit, I've removed passing the QueryCache to the createStore or the set function, which I think is the best way api wise.

Yes you have internal circular dependency and i think it's ok, but exposing circular dependency in userland is what i think you should try to evoid.
for internal circular deps, you can fix the issues, but once it's exposed to users, you will potentially break their code and they will complain.

Yes you have internal circular dependency and i think it's ok, but exposing circular dependency in userland is what i think you should try to evoid. for internal circular deps, you can fix the issues, but once it's exposed to users, you will potentially break their code and they will complain.

Hmm yeah we are exposing the dependency by making a public getter on the query that returns the cache:

• https://github.com/TanStack/query/pull/4726/files#diff-b5729917de14b1b87b04fa82d78a8abcafd81972cc9af9e8586ef27527d77ebbR178-R180

what could that break in your opinion? Note that the fields were only typescript private, so at runtime, they were always there ...

@TkDodo

Proposed solution

So exposing the cache linked to the query means users will for sure use it now.
Will this create a burden for you to maintain react-query when you will want to refactor your internals?
Any refactor on this part will mean a breaking change, so limit your refactoring to major versions.
This is all hypothesis, so maybe not a concern for now. But for example, in the future, to enable some new features, you may want to break you internal cyclic dependency (Big if). If you expose it, you'll need to postpone the feature to a major version.
Maybe it's fine.

Another approach

So the current solution is to plug a custom store to the cache.
Maybe it's the wrong abstraction. Maybe the solution could be to simply allow users to plug an alternative QueryCache directly to QueryClient.
For this you could define the public interface of a QueryCache and also define the public events a QueryCache needs to emit for the plug-in to work.
To help users to bootstrap a QueryCache effitiently, you could simply provide an event emitter that already has all the events setup.
This way custom cache will use the provided emitter, and only implement the business Logic.

What do you think about this second approach ? If it's not clear, i'm all for setting a call on discord if you want.

So I worked on this today with @ecyrbe and we found out that size limitation of the cache can actually be achieved better by subscribing to the cache, as shown in this test:

since we couldn't come up with a good use-case that actually needs the custom store, we've reverted the change for now (but kept the refactoring towards an internal map). We can expose this feature in a future minor if we want to.