Feature/stale time on query #8313
Conversation
as it doesn't make much sense to have different stale times for the same query
this was part of isStale() before, so it got lost in the refactoring
we use `isValidTimeout` in 3 places: - gcTime: here, we want 0 to trigger a setTimeout, because otherwise, we don't cleanup - staleTime: 0 should be invalid because with 0, we instantly mark queries as invalidated - refetchInterval: 0 was never a valid timer (it's treated the same as false); we had an exception implemented here 2/3 cases want it to _not_ be valid, so that should be the default
options on query level have never been merged with previous options, so not passing staleTime now makes things stale (0)
given that a query is currently stale, a new staleTime that is > 0 should set it back to being not-stale
…a lower one where the lower number actually makes the query instantly stale also, isStale() will always return true if we have no data yet (duh)
to achieve that, we move the "early return" until after we've dispatched; all the logic in between also works with staleTime: Infinity - timeUntilStale just returns Infinity as well (added tests for that, too)
setOptions doesn't merge, so we need to call it at the specific places where we want it to overwrite - fetchQuery and ensureQueryData
☁️ Nx Cloud ReportCI is running/has finished running commands for commit fcea9bf. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
Note: I didn’t fix the other framework adapters yet - only the core and react, so that’s why the tests are failing. |
…ructor that way, this.state.isInvalidated is set correctly for staleTime: 0
| const nextStaleTime = this.#resolveStaleTime(this.options.staleTime) | ||
| if (nextStaleTime === 0) { | ||
| this.#initialState.isInvalidated = true | ||
| } | ||
|
|
||
| this.#updateStaleTimeout(nextStaleTime) |
There was a problem hiding this comment.
#resolveStaleTime needs access to this.state (because it passes the query to the functional syntax of staleTime, so we need to do this after we have set this.state.
However, this.state might be initiated from the #initialState. While this.#updateStaleTimeout will make sure that this.state.isInvalidated is reflected correctly, we have to “correct” the #initialState here.
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
| const prevStaleTime = this.options?.staleTime |
There was a problem hiding this comment.
this is called from the constructor where this.options isn’t initialized yet. Technically, this.options needs to be optional on type level, but it will be set for every other place so this feels like the best hack.
| const nextStaleTime = this.#resolveStaleTime(nextOptions?.staleTime) | ||
|
|
||
| // Update stale interval if needed | ||
| if (nextStaleTime !== this.#resolveStaleTime(prevStaleTime)) { | ||
| this.#updateStaleTimeout(nextStaleTime) | ||
| } |
There was a problem hiding this comment.
when the staleTime option changes, we need to update our timer
| isStale(): boolean { | ||
| if (this.state.isInvalidated) { | ||
| return true | ||
| } | ||
|
|
||
| if (this.getObserversCount() > 0) { | ||
| return this.observers.some( | ||
| (observer) => observer.getCurrentResult().isStale, | ||
| ) | ||
| } | ||
|
|
||
| return this.state.data === undefined | ||
| } | ||
|
|
||
| isStaleByTime(staleTime = 0): boolean { | ||
| return ( | ||
| this.state.isInvalidated || | ||
| this.state.data === undefined || | ||
| !timeUntilStale(this.state.dataUpdatedAt, staleTime) | ||
| ) | ||
| return this.state.isInvalidated || this.state.data === undefined | ||
| } |
There was a problem hiding this comment.
this is beautifully simple now
| const nextStaleTime = this.#resolveStaleTime(this.options.staleTime) | ||
| if (nextStaleTime === 0) { | ||
| this.state.isInvalidated = true | ||
| } else if (!this.isStale()) { | ||
| this.#updateStaleTimeout(nextStaleTime) | ||
| } |
There was a problem hiding this comment.
some state updates might re-set this.state.isInvalidated, but if our staleTime is zero, we are basically always stale, so we undo that here. Also, the timer needs to be updated because after the reducer, our dataUpdatedAt might have changed.
| if (this.state.isInvalidated !== newInvalidated) { | ||
| this.#dispatch({ | ||
| type: 'setState', | ||
| state: { isInvalidated: newInvalidated }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
this will “toggle” the isInvalidated state to what it needs to be depending on the new timer we’re setting (or not setting)
| isInvalidated: false, | ||
| isInvalidated: !hasData, |
There was a problem hiding this comment.
this change is not strictly necessary, because a query is always considered stale if we have no data, but it’s also technically more correct
| query.setOptions(defaultedOptions) | ||
|
|
||
| return query.isStaleByTime( | ||
| resolveStaleTime(defaultedOptions.staleTime, query), | ||
| ) | ||
| return query.isStale() |
There was a problem hiding this comment.
one of the more tricky things is that we now need to call setOptions for fetchQuery and ensureQueryData because they accept staleTime as an option, but they didn’t update the options, so calling query.isStale() wouldn’t reflect that.
I think this is also correct because if you pass different settings here, like a gcTime, you would want that to be reflected.
Note that query.fetch will set the options for us under the hood, but it’s not guaranteed that it is invoked, because we might just return cached data from `fetchQuery. This is likely also an edge case bug in the current version 😅 .
There was a problem hiding this comment.
Reflected on this a bit more, and I don’t think the change here is correct. When we say:
queryClient.prefetchQuery({ queryKey, queryFn, staleTime: 10 * 60 * 1000 })
we basically want to express: prefetch this query if data is older than 10 minutes.
it doesn’t mean we want to set the staleTime of the query to 10 minutes. For example, if there is an active observer that has a staleTime of 2 minutes - that shouldn’t change.
So I need to revert that change and instead, somehow revive the isStaleByTime check for the queryClient methods.
| !isValidTimeout(this.#currentRefetchInterval) || | ||
| this.#currentRefetchInterval === 0 | ||
| !isValidTimeout(this.#currentRefetchInterval) |
There was a problem hiding this comment.
note: timeout 0 is now not valid per default, so we can remove the extra check here for refetchInterval, too.
| @@ -11,7 +11,7 @@ export abstract class Removable { | |||
| protected scheduleGc(): void { | |||
| this.clearGcTimeout() | |||
|
|
|||
| if (isValidTimeout(this.gcTime)) { | |||
| if (isValidTimeout(this.gcTime) || this.gcTime === 0) { | |||
There was a problem hiding this comment.
note: this was the only place where we wanted 0 to be a valid timeout, so I re-added the extra check here. Otherwise, gcTime 0 would not garbage collect.
|
@TkDodo this is the most important thing here:
You have simplified a core concept while retaining essentially the same API. This will make maintaining and using TSQ easier. The more you can remove or rip out, the better imo. Stay focused, stay lean. |
DO NOT MERGE, contains potentially breaking changes
This PR moves the
staleTimehandling fromQueryObserverto theQueryitself, which mostly disallows differentstaleTimeson the same Query while being mounted simultaneously. Note that we can still have differentstaleTimesfor different screens.How it worked before
Every observer had a
staleTime, and every observer also triggered a timer. When the timer was done, we triggeredcreateResult()for the observer, which would updateisStaleon it. This means multiple timers were running - one for each observer - which could cause performance problems once you hit a certain threshold, at about 1k observers for the same query. Largely, those observers all have the samestaleTime, so using one timer would be a lot better.Additionally, I think this implementation lead to cases where one observer had
isStale: truewhile another observer could haveisStale: false. But conceptually,datais either stale for a screen or it isn’t. It can’t be both, because that would mean we would need to show inconsistent data, which we can’t. As soon as one observer is marked as “stale”, a smart refetch would re-fetch data, thus showing it for all observers. So this isn’t really a case we should be supporting.How it works now
staleTimemoved fromQueryObserverOptionstoQueryOptions. You can still pass it touseQueryor define a global default - nothing changed here from user perspective. But this means we’ll havestaleTimeon theQueryitself, and since there can be only one Query per key, we’ll also only have one timer.The timer will update itself when new options come in, or, when new data comes in for the query.
When the timer elapses, it will just set the
isInvalidatedflag on the Query itself totrue. This is the same flag that we use forqueryClient.invalidateQueries. Conceptually, I think this is neat because we don’t really need to distinguish between a Query that has been marked as “invalid” because it was invalidated by the user programmatically, or because the timer has elapsed.Setting this flag will then inform all observers, so all screens are always up-to-date.
staleTime: Infinitywill just not set a timer.What’s also neat about this is that we can now persist that setting, because it’s part of the query state. When restoring from localStorage, we already know if that data is stale or not. This wasn’t the case before because we would only know as soon as the first observer mounted.
Alternative design: separate state
Note that I did consider not re-using the
isInvalidatedflag to keep the distinction between manual invalidations and timer-based ones, and use anisStaleflag instead. I don’t think it is necessary, and it would actually lead to invalid states (yay, booleans), so we would need an enum, something like:isStale: 'invalidated' | 'timer' | falseand I don't necessarily like that here / want to do a breaking change for that.staleTime: 0, a special case
Additionally, I’ve changed how
staleTime: 0works, because it’s a special case. Previously, 0 was considered a valid timeout, and we set asetTimeout(1)on the observer, which would then trigger the query to be marked as stale (because we have to add +1 to the timeout for edge cases). This was pretty unnecessary - when staleTime is zero, we want the query to be stale immediately, without a timer.So I added some special handling for this: When the query is created with staleTime zero, we set the
initialStatetoisInvalidated: true. Also, whenever the query updates, we set it toisInvalidated: truefor thatstaleTimeimmediately, without setting a timer.Lastly, when the timeout changes and it would result in a stale query, we also update
isInvalidated. Having a query with data from 10 seconds ago with a staleTime of 1 minute will therefore give usisStale: false, but if we update thestaleTime(e.g. because it’s a function depending ondata) to be 1 second, we will immediately transition toisStale: true)This refactoring got rid of a lot complexity: We used to have
isStaleandisStaleByTimefunctions on thequeryand and additionalisStalefunction on the observer. Now, it’s just:which is beautiful. Note that queries without data are always stale, this was the same before too (this is largely for the
errorcase).disabled observers
Disabled observers were previously exempt from being stale; I made that because of some issue of what shows up in the devtools, but I think that was a mistake. Observers just show
data, and the stale-ness refers to the data. Disabled observers also show data and update if that data changes, so it’s important to reflect that in theisStaleproperty of those. This is where the majority of test updates comes from.why this could be breaking
staleTimeson the same query with two observers mounted at the same time, because it doesn’t make sense anymore with this implementation. If users do this, they might see changes in behaviour.isStale: trueimmediately if they start out withstaleTime: 0, while previously, they might have started withisStale: falsefollowed by an immediate update.All of these could be considered fixes as well 😅