Closed ali-idrizi closed 1 month ago
Thanks for reporting 🙏
The first one with staleTime
is definitely wrong, we need to look into this.
As for the second one, that’s also an issue 😂 . When we go to a new Query with a key that has no data, we need to suspend again. You’re absolutely right that you would need deferred values, transitions or our placeholderData: keepPreviousData
feature to keep “old data” from a different key on the screen.
@KATT FYI.
here is a failing test case for the first one:
it('should show correct data when switching between cache entries without re-fetches', async () => {
const key = queryKey()
function MyComponent(props: { promise: Promise<string> }) {
const data = React.use(props.promise)
return <>{data}</>
}
function Loading() {
return <>loading..</>
}
function Page() {
const [count, setCount] = React.useState(0)
const query = useQuery({
queryKey: [key, count],
queryFn: async () => {
await sleep(10)
return 'test' + count
},
staleTime: Infinity,
})
return (
<div>
<React.Suspense fallback={<Loading />}>
<MyComponent promise={query.promise} />
</React.Suspense>
<button onClick={() => setCount(count + 1)}>inc</button>
<button onClick={() => setCount(count - 1)}>dec</button>
</div>
)
}
const rendered = renderWithClient(queryClient, <Page />)
await waitFor(() => rendered.getByText('loading..'))
await waitFor(() => rendered.getByText('test0'))
fireEvent.click(rendered.getByText('inc'))
await waitFor(() => rendered.getByText('test1'))
fireEvent.click(rendered.getByText('dec'))
await waitFor(() => rendered.getByText('test0'))
})
It’s also interesting / wrong that we don’t suspend in between queries. When going from test0
to test1
, it’s a new cache entry, so we should see the suspense boundary again. That is, unless we would use placeholderData
or useTransition
.
But as it stands, if I add this as an assertion, it will also fail:
fireEvent.click(rendered.getByText('inc'))
+ await waitFor(() => rendered.getByText('loading..'))
await waitFor(() => rendered.getByText('test1'))
I think #8169 should fix both reported cases - please give it a try @ali-idrizi. I’ve also added two test cases for your two reported findings
Hey @TkDodo, thanks for the update. I just checked this out and it's working much better! However, I still noticed an issue with incorrect data being shown. Here's a screen recording:
https://github.com/user-attachments/assets/13a25d79-1039-4e06-8e1c-31a6016f6712
I have pushed that example in a repo: https://github.com/ali-idrizi/rq-stale-invalid-promise-reproduction.
This seems to occur only when typing faster in the input, and multiple promises are pending. I not super familiar with the inner workings of everything here, but to me it looks like this is an issue with React itself? Looking at the logs:
<Search> requested query: "1". received promise: pending undefined
<Search> requested query: "12". received promise: pending undefined
<Search> requested query: "123". received promise: pending undefined
<Search> requested query: "1234". received promise: pending undefined
<Search> requested query: "12345". received promise: pending undefined
<Search> requested query: "123456". received promise: pending undefined
<Search> requested query: "123456". received promise: fulfilled {q: '123456'}
The correct promise was received for every single query. However, in the recording the <Data>
component was rendered multiple times for an incorrect q
, even though the parent <Search>
component received a fulfilled promise only once.
I tested further by instead having the <Data>
component get the promise
directly from useQuery
instead of the props (like it's shown in the docs), and in that case it works perfectly fine. So in the repo:
- // const response = use(useData(q).promise);
- const response = use(query.promise);
+ const response = use(useData(q).promise);
+ // const response = use(query.promise);
@ali-idrizi thanks for trying out this feature and reporting the feedback - I really appreciate it ❤
I think I have a fix for what you are reporting here:
Basically, we were resolving with data from a previous query once it resolves, which explains the intermediate data flashes. I think this was introduced with my latest refactoring, because createResult
is called way more often than updateResult
.
I still need to write a test for it, but maybe you can try out the preview build from that PR in the meantime and let me know if that fixes it? All other tests are still passing so I hope there is no regression (at least of paths that we are aware about):
pnpm add https://pkg.pr.new/@tanstack/react-query@8171
@TkDodo You're welcome! I'm glad to help out. I tested the new change, and it works much better when typing now.
The only potential regression that I noticed, is that it's now suspending briefly when it has cached data. I suppose in this case a fulfilled promise should be provided instead of a pending one which resolves later (that was the case previously)?
Here's a screen recording where you can see that as I remove characters from the input:
https://github.com/user-attachments/assets/599aa904-0d52-4fae-8ea6-997dbba765ad
yeah I’ll look into that too
ok, got it; this was introduced by my latest attempts to improve things 😂
I pushed another commit and more tests. can you try again?
latest commit is: https://pkg.pr.new/@tanstack/react-query@a4a2cf7
Perfect 🚀. I'll try it out on a larger repo next week, and report if I find any other issues.
alright, shipped it so that you don’t have to use preview builds for that :) enjoy 🍻
@TkDodo I looked a bit more into this, and perhaps there's one more issue. I am not sure whether what I am doing is the intended usage, so this is more of a question.
The docs show the query
is being passed as a prop to the child component, and then the promise
is extracted from it. That still works well! But, in a larger repo, the data might be needed quite deep in the tree or by many components, so calling the useQuery
again to get the promise
would be favored.
What I did in the <Search>
component is simply call useData(q)
, so it can start prefetching (not doing anything with the results, so nothing being passed to <Data>
). In the <Data>
component, I am calling the same useQuery
again, and getting the promise
from there:
Search.js
function Search() {
const [q, setQ] = useState("");
useData(q); // `useData` simply calls `useQuery` and returns everything
return (
<>
<input value={q} onChange={(e) => setQ(e.target.value)} />
<Suspense fallback={<div>suspending...</div>}>
<Data q={q} />
</Suspense>
</>
);
}
Data.js
function Data({ q }) {
const query = useData(q);
const data = use(query.promise);
return ...;
}
However, that works well only for the first ever fetch, if I change the q
it gets stuck suspending:
https://github.com/user-attachments/assets/a1db0d69-14d4-4336-a8cb-b8ffefbca1b7
If you look at the devtools you can notice that [""]
for some reason still has one observer, and ["5"]
also has one instead of two. I have no idea what's happening there.
If this could work it would make getting the promise
much easier, but I understand if this is not be the intended usage. Just let me know either way :)
I'll look into this as well, probably on Monday.
took a bit longer but I found it:
Thanks for the update. I tried out the latest release and that is working great now!
I came across another case that suspends indefinitely. It happens consistently when the promise and the suspense are colocated, but the promise fails. I modified one of the existing test, it currently fails waiting for the error boundary. I have filed a new issue for better tracking: https://github.com/TanStack/query/issues/8219.
Describe the bug
I was trying out the new
useQuery().promise
implementation with theReact.use()
hook, and noticed two issues with it.For the reproduction I have a very simple setup similar to the docs. There is an input that updates the
q
stateonChange
, and the following query:There's also a
Data
component wrapped in aSuspense
, which take the promise of the query above as a prop and callsuse
on it. The query function takes theq
, delays 1s and returns the same:The first issue occurs when
staleTime
is set, and it still has fresh data. In this case it seems that the promise in not being restored, and incorrect data is shown. Here's a screen recording:https://github.com/user-attachments/assets/101dd91e-fdb4-433e-85a8-4e897172821f
When typing extra characters, it correctly updated the data. However, when I remove characters (in which case it already had fresh data cached) it doesn't seem to restore the correct promise.
The devtools is showing the correct query as active, and the
Data
component is re-rendering. but from the console logs it seems to be receiving an incorrect promise.This issue does not occur when the
queryFn
is ran, e.g. when providing a query that is not cached or whenstaleTime: 0
.And the second one, the suspense is only triggering once, and not when the query changes. In the screen recording you can notice as I type "12345", it no longer triggers the suspense (which has "suspending..." as fallback), but it provides a stale promise, until it's done fetching the data for the new query.
I was expecting the suspense to trigger when the query key changes and it's about to fetch data. I thought the current behavior (i.e. show stale data until the new render is done) would have to be achieved through
useDeferredValue
instead.I am not sure if this is the intended behavior, so I am did not create a new issue for it, but please let me know if I should do that.
Your minimal, reproducible example
https://codesandbox.io/p/sandbox/awesome-williamson-k2269m
Steps to reproduce
On the first issue, simply type anything in the input and notice the data under it updating. Now remove characters and notice that incorrect data is shown.
On the second issue, notice the suspense triggering when reloading the page, but not when changing the query (in which case
query.isFetching
is indeedtrue
).Expected behavior
Already explained above.
How often does this bug happen?
Every time
Screenshots or Videos
No response
Platform
Tanstack Query adapter
react-query
TanStack Query version
5.59.0
TypeScript version
N/A
Additional context
No response