-
Notifications
You must be signed in to change notification settings - Fork 635
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
Add createQueryStore #6325
base: develop
Are you sure you want to change the base?
Add createQueryStore #6325
Conversation
… store methods from persisted state
…ocs, add overloads, misc. fixes
…each fetch operation
…uracy, make fetch return data, expose params to transform, misc. optimizations
…ignal equalityFn, bump fast-compare
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected] |
…ng a failed fetch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really nice.
Only concerns are around the functionality of cacheTime
and staleTime
, but it's kind of subjective. In general, I think how those behave should stay closer to how they behave in react query to make transitioning from it simpler. A short staleTime
can be useful when you know a component might get mounted / unmounted in a short enough timespan that you wouldn't want to trigger another fetch, but in those scenarios you don't want to also have it refetching at that same time interval.
For cacheTime
I would expect that after the cacheTime
has elapsed that the cached data would no longer be returned. Right now the cache isn't pruned until after the fetch, which means the component would get expired data until the fetch was complete. There should be some way to evict the cache after it is expired to prevent it from being returned when the initial fetch is happening.
For staleTime
, I do think it should be split into a separate staleTime
and refetchInterval
to make it simpler to opt out of refetches while still maintaining control of when the data should be considered stale.
const [persist, discard] = [true, false]; | ||
|
||
const SHOULD_PERSIST_INTERNAL_STATE_MAP: Record<string, boolean> = { | ||
/* Internal state to persist if the store is persisted */ | ||
enabled: persist, | ||
error: persist, | ||
lastFetchedAt: persist, | ||
queryCache: persist, | ||
queryKey: persist, | ||
status: persist, | ||
|
||
/* Internal state and methods to discard */ | ||
fetch: discard, | ||
getData: discard, | ||
getStatus: discard, | ||
isDataExpired: discard, | ||
isStale: discard, | ||
reset: discard, | ||
subscriptionCount: discard, | ||
} satisfies Record<InternalStateKeys, boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think assigning variables here adds any clarity over just assigning true or false directly
const SHOULD_PERSIST_INTERNAL_STATE_MAP: Record<string, boolean> = {
/* Internal state to persist if the store is persisted */
enabled: true,
error: true,
lastFetchedAt: true,
queryCache: true,
queryKey: true,
status: true,
/* Internal state and methods to discard */
fetch: false,
getData: false,
getStatus: false,
isDataExpired: false,
isStale: false,
reset: false,
subscriptionCount: false,
} satisfies Record<InternalStateKeys, boolean>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
…dant work when multiple subscribed components are mounted, reduce subscription handler redundancy, expose more tools to onFetched, clean up lastFetchedAt logic, other small improvements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. I need to test closer for potential memory leaks with timeouts etc. but first glance looks fine.
const [persist, discard] = [true, false]; | ||
|
||
const SHOULD_PERSIST_INTERNAL_STATE_MAP: Record<string, boolean> = { | ||
/* Internal state to persist if the store is persisted */ | ||
enabled: persist, | ||
error: persist, | ||
lastFetchedAt: persist, | ||
queryCache: persist, | ||
queryKey: persist, | ||
status: persist, | ||
|
||
/* Internal state and methods to discard */ | ||
fetch: discard, | ||
getData: discard, | ||
getStatus: discard, | ||
isDataExpired: discard, | ||
isStale: discard, | ||
reset: discard, | ||
subscriptionCount: discard, | ||
} satisfies Record<InternalStateKeys, boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
if (IS_DEV && !suppressStaleTimeWarning && staleTime < MIN_STALE_TIME) { | ||
console.warn( | ||
`[createQueryStore${persistConfig?.storageKey ? `: ${persistConfig.storageKey}` : ''}] ❌ Stale times under ${ | ||
MIN_STALE_TIME / 1000 | ||
} seconds are not recommended.` | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should just override the value if it's beneath this value. I don't know if you see a reason for having it exist, but seems to me that it should be disallowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to create a query that fetches freshest data with staleTime = 0
, but that doesn't need to be refetched on an interval by setting disableAutoRefetching
const abortController = new AbortController(); | ||
activeAbortController = abortController; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to create a local variable here instead of just assigning directly to activeAbortController
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used here to make sure that activeAbortController
is only nulled out if it hasn't been reassigned:
rainbow/src/state/internal/createQueryStore.ts
Lines 486 to 490 in f0d4737
} finally { | |
if (activeAbortController === abortController) { | |
activeAbortController = null; | |
} | |
} |
Fixes APP-2154
What changed (plus any additional context for devs)
createQueryStore
, which is a version ofcreateRainbowStore
with built-in fetching functionality, designed to be a more performant, more composable alternative touseQuery
, for most use casesuseQuery
→ Zustand sync pattern, which is inefficient and redundantMap
andSet
persistence support tocreateRainbowStore
partializer
tocreateRainbowStore
that automatically omits top-level store methods, which previously were being persisted whenpartialize
was not specifiedHow It Works
zustand-signal
. The$
function transforms a slice of a Zustand store into a liveAttachValue
. Under the hood, it’s a proxy that subscribes to changes in that slice of the store's state. When the specified state updates, the proxy immediately detects this, forcing the query store to recompute its internalqueryKey
based on the new params and refetch if needed.$
APIQueryStoreConfig
interfaceQueryStore
state interfaceNotes
set
andget
within the custom state creator currently are not aware of the query store's internal stateMay want to add a helper setting for a manual mode, where no automatic refetching would occur (perhaps with the exception of the initial fetch), relying only on manually callingfetch
for data updatesCurrently you can achieve something close to this by specifyingstaleTime: Infinity
(first-time fetches will still occur)disableAutoRefetching
API Examples
The API In Its Simplest Form:
Dynamic
$
Params:Narrow state subscriptions linked directly to either internal or external store state.
No Params & Overriding Built-in Data Caching via
setData
:Screen recordings / screenshots
Two copies of the same query store implementation, each with their own state and a unique
address
param, both leveraging persistence and dynamic$
params (you wouldn't want two copies of the same store in practice, that's purely for testing purposes):ScreenRecording_12-23-2024.18-02-12_1.MP4
What to test