You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When implementing #1992, I've made a list of possible improvements/fixes to our RPC and caching story:
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
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.
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).
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[].
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?
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).
The text was updated successfully, but these errors were encountered:
When implementing #1992, I've made a list of possible improvements/fixes to our RPC and caching story:
query.queryCacheKey
has a confusing name - The publicQuery
object includes a fieldqueryCacheKey
. Unlike its name suggests, this property is not truly a fullqueryCacheKey
. 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 misleadingDuplication on the Query object - The Query object contains two properties:
queryCacheKey
(see above).route
used for communicating with the backend.These two properties are coupled: both
queryCacheKey
androute
can be unambiguously constructed from therelativeQueryPath
. Since we should renamequeryCacheKey
to something else anyway (and perhaps even swap it forrelativeQueryPath
), we should also reconsider howroute
fits into this story.Duplication of key construction logic - We construct the full
queryCacheKey
in three places:createQuery
function anduseQuery
hook usemakeQueryCacheKey
.getRqQueryKeyFromSpecifier
.Let's find a way to remove this duplication (perhaps by attaching the key calculation logic to the Query object).
The
QuerySpecifier
type is underspecified (no pun intended :)) - TheQuerySpecifier
consists of aQuery
object and its payload. TheQuerySpecifier
type should use the proper payload type instead ofany[]
.The
UpdateQuery
type should be allowed to returnundefined
- If the cache was empty, we might want to set it back to empty (i.e., returnundefined
). Things to double-check:undefined
?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).
The text was updated successfully, but these errors were encountered: