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

Revisit our client cache implementation #2017

Open
sodic opened this issue May 7, 2024 · 0 comments
Open

Revisit our client cache implementation #2017

sodic opened this issue May 7, 2024 · 0 comments
Labels
enhancement New feature or request refactoring Keeping that code clean!

Comments

@sodic
Copy link
Contributor

sodic commented May 7, 2024

When implementing #1992, I've made a list of possible improvements/fixes to our RPC and caching story:

  1. query.queryCacheKey has a confusing name - The public Query object includes a field queryCacheKey. Unlike its name suggests, this property is not truly a full queryCacheKey. It is only a part of it. The whole key is constructed dynamically on each call and depends on the Query's payload. This name is, therefore, pretty misleading

  2. Duplication on the Query object - The Query object contains two properties:

    • The inappropriately named queryCacheKey (see above).
    • The route used for communicating with the backend.

    These two properties are coupled: both queryCacheKey and route can be unambiguously constructed from the relativeQueryPath. Since we should rename queryCacheKey to something else anyway (and perhaps even swap it for relativeQueryPath), we should also reconsider how route fits into this story.

  3. Duplication of key construction logic - We construct the full queryCacheKey in three places:

    • The createQuery function and useQuery hook use makeQueryCacheKey.
    • Optimistic updates use getRqQueryKeyFromSpecifier.

    Let's find a way to remove this duplication (perhaps by attaching the key calculation logic to the Query object).

  4. The QuerySpecifier type is underspecified (no pun intended :)) - The QuerySpecifier consists of a Query object and its payload. The QuerySpecifier type should use the proper payload type instead of any[].

  5. The UpdateQuery type should be allowed to return undefined - If the cache was empty, we might want to set it back to empty (i.e., return undefined). Things to double-check:

    • Does it matter semantically whether the cache is not defined or set to undefined?
    • Do we maybe want to do some of the stuff mentioned here?
  6. The types for optimistic updates can be improved - Just revisit the entire thing. That type assertion in userland is especially problematic since it removes many checks (e.g., on return values).

@sodic sodic added enhancement New feature or request refactoring Keeping that code clean! labels May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactoring Keeping that code clean!
Projects
None yet
Development

No branches or pull requests

1 participant