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 ce2847c. 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.
There was a problem hiding this comment.
So I tried that, and it sadly led to another can of worms. If we try to re-implement isStaleByTime, we have a problem with manual invalidation. The fetchQuery docs say:
If the query exists and the data is not invalidated or older than the given staleTime, then the data from the cache will be returned. Otherwise it will try to fetch the latest data.
The problem is that, by design, we no longer distinguish between invalidated and stale - the stale timer sets it as invalidated. So I think treating staleTime as any other option of the query, and doing the setOptions call, is okay. What we get in return is that other query options are correctly updated even if no fetch happens. All of this is theoretical of course, because we always encourage users to abstract options into either custom hooks or queryOptions, so I think none of this matters in practice.
| !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. |
…QueryOptions, because staleTime is now an option on the query anyway
|
First off, let me start by staying I agree with the goal of this PR, having a single timer for a query would be a great thing! I love the clear description and comments you've left. Let me also be clear that right now I don't have an opinion yet whether the approach in this PR is correct or not. Also I have yet only skimmed the technical details, to stay unbiased for this next part.
|
|
This makes sense and should simplify things. 🚀 One general note though is about current usage, and potential misuse by users. Considering that
I imagine this can be a problem even now. |
This is a problem in the new implementation, yes, and the test I deleted was actually failing because of that 😂 . Well spotted. It’s not an issue in the current implementation because each observer has its own timer, and when the timer elapses, it only updates the result of this observer. Nothing in the query is updated, so there are no chances to influence other observers. After reading @Ephem’s comment, and talking about this with him some more, I think I want to try a different approach:
|
My thought process would be: since a disabled observer can’t trigger fetches on its own, it also shouldn’t influence when that data is considered “outdated”, so it should be excluded from deriving the staleTime value.
for this, I would say YES. also, disabled observer should get that update. If we treat “disabled observers not getting the correct and would be fixed implicitly with this proposal, too. |
When i think about
I would say, it should say |
seems we are aligned on that 👍
I also agree with that
I also added a dev warning, but @Ephem is skeptical about it, and I kinda agree. Imagine a scenario where you have two observers on the same query: one selects the This is fine, but now you refactor a bit and put the counter also in the header of the list page. Yes, this would make the whole list get the smaller staleTime, but is it worth warning about? The only way out would be to make that component customizable to take in a different staleTime, so you have them be the same on the same screen. It gets worse if you have a component that opens in a dialog that sets a different staleTime, because that’s only for the time when that dialog is opened. Finally, in React Native, screens might still be mounted even though they are not visible, so you might really have multiple observers with different staleTimes mounted. That could be seen as a config error, but we actually want to ship an improvement to properly disconnect those observers (see #8348). All in all I don’t think a warning in dev mode is warranted, but it’s not hard to implement and we can think about it. The things we log in dev mode can’t be turned off (because they should be actionable), and I don’t think these warnings would be something that the user can or should act upon. |
|
Another interesting question that @Ephem had: If we have two observers, one with BUT, what if another observer with
I think the answer should still be YES, just given by the fact that we will return Just wanted to note down my thoughts on it while they are still fresh 😂 |
|
@TkDodo your last comment confused me slightly, Pls correct me where/when I'm wrong or don't have the concept clear. Restate concepts: I think of staleTime as declarative, invalidation as imperative, and staleness as derived from staleTime. I will simplistically think of observers as instances of useQuery calls (with or without a selection), and queries as the keyed query function we all know and love. Key Question: Should staleness be exclusively a property of queries, observers or actually both? To retain the utility of expressing selections of the same query having different priority & use in an app, I think staleness should be exclusively a property of observers. IE a query cannot be stale, only observers of queries can be stale. On StalenessI find this approach of observers only having staleness simple and useful. It does not This may be false or wrong in ways I currently don't understand. Restated scenario:
I will rephrase your question, instead of refetch on mount, I would simply ask - what is stale when our third observer is mounted? If staleness is only a property of observers, then it is easy imo. In my sub-scenario a, the active observer, the third one, is not stale so we don't refetch, we can serve from cache and respect what the author has declared via staleTime. In my sub-scenario b, we have this new 'mount' event, and we have an active observer that has declared staleness, so we could use this as an opportunity to refetch so we respect the first observer's staleness. So maybe both? Buuut.. consider that the new component is a descendant, do we want some deep descendant who is mounting cause a parent to get new data? Actually, probably not. If we navigated away and came back, then yes I would expect fresh data. But if I'm revealing more information on a sub tab or table or expander, I wouldn't expect my top level nav to parent to get fresh information unless I configured it that way, and perhaps there isn't a way to configure that scenario? This makes me think that in the case of a "far away" observer being mounted, we may want a new option in QueryOptions to mark an observer as "eager" (is this already a thing?) so if this scenario happens, TSQ always iterates and takes the minimum of all active observers to see if anyone wants fresh data, and if so it refetches. This may cause some weirdness though, we can technically serve stale data to the third component while refetching, and then it could update. But I guess that's what isRefetching is for anyways so nvm (hence my "eager" or opt in suggestion). Sorry for the essay, here's a picture of one of our catsThis is Pippin and he gets fed because I and all our teams use TSR and TSQ everyday <3.
|
|
@TkDodo I'm now realizing your scenario of a window focus event is like going back to the tab after getting distracted on youtube (not me ofc), not navigating in an app. In which case the logic above would say "YES" you have a stale observer of the shared query and need to refetch. edit: no i think i was right in my first interpretation actually, unless you've combined the window focus with the third mounting? instructions unclear, going to youtube. |
|
@snewell92 thank you for this, really, there are good insights here. I think we are mostly aligned on what we want, namely:
This is spot on 👍
Yep 👍
Also 👍 . What’s currently on Regarding the scenario:
Since the first two observers aren’t on the new page, they are irrelevant, because they get unmounted. So on this new page, there is only one observer with
Now we have 2 observers when we add a 3rd one, so we take the minimum It might seem a bit confusing that we get a background refetch here even though the Does that makes sense? |
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 😅