TanStack / router

🤖 Fully typesafe Router for React (and friends) w/ built-in caching, 1st class search-param APIs, client-side cache integration and isomorphic rendering.
https://tanstack.com/router
MIT License
7.73k stars 564 forks source link

Upgrade from 1.45.2 to 1.45.3 changes `isLoading` state for persisted tanstack query #1983

Closed RobertOstermann closed 2 weeks ago

RobertOstermann commented 1 month ago

Describe the bug

I use the PersistQueryClientProvider with Tanstack Query to cache query data in local storage.

In v1.45.2, the initial isLoading state of the query is true if the data needs to be refetched.

In v1.45.3, the initial isLoading state of the query if false, then updates to true, instead of starting with true.

Your Example Website or App

https://stackblitz.com/edit/github-fucdao?file=package.json

Steps to Reproduce the Bug or Issue

  1. Go to https://stackblitz.com/edit/github-fucdao?file=package.json
  2. Run the project, notice the isLoading state begins (on refresh) with false, before updating to true, and finally returning to false with the data fetched.
  3. Update the package.json
    1. Change @tanstack/react-router to 1.45.2
    2. Change @tanstack/react-devtools to 1.45.2
    3. Change @tanstack/router-plugin to 1.45.2
  4. Restart the vite server to optimize for the dependency changes
  5. Run the project, notice the isLoading state begins with true on any refresh

Expected behavior

I expect the isLoading state to begin with true if data needs to be fetched.

Screenshots or Videos

No response

Platform

Additional context

No response

schiller-manuel commented 1 month ago

I played around with your example, interestingly you can get the same behavior with 1.45.3 just by defining a loader like this:

  loader: () => {},

https://stackblitz.com/edit/github-fucdao-4mlrqa?file=src%2Froutes%2Findex.tsx

tannerlinsley commented 1 month ago

How sure are we that stackblitz is properly managing that persisted query client?

Technically, I would expect this new experience to be accurate, since regardless of the cache, the first time we render the component, the useQuery() hook hasn't executed any of it's side effects yet, so isFetching has not started until immediately after the first render. As to why the previous functionality was initially rendering isLoading: true as the initial value, that's really really strange.

The only reason a query would render with isLoading: true initially, would be due to some non-react logic (like a loader) kicking it off, or a previously suspended parent render kicking it off.

Even for the PersistQueryClientProvider, it's safe to say that no loading or queryClient side effects would happen until after the first render (because of the useEffect) anyway: https://github.com/TanStack/query/blob/main/packages/react-query-persist-client/src/PersistQueryClientProvider.tsx#L28-L49

I feel like I could be missing something, but please tell me if what I just said doesn't make sense.

RobertOstermann commented 1 month ago

Stackblitz has the same behavior my project did, so at least in the loading states it is behaving the same.

That makes sense. I can move my loading logic over to loaders to ensure the data is present on the first render. I was just surprised by the change and wasn't sure if it was a bug or if it was intentional. It also seems odd to me that defining an empty loader as @schiller-manuel described also reverts the behavior to set isLoading: true as initial value.

RobertOstermann commented 3 weeks ago

@tannerlinsley When attempting this with the regular QueryClientProvider instead of the PersistQueryClientProvider I notice the loading state begins with isLoading: true. Do you know why the behavior differs between the two?

I think the previous PersistQueryClientProvider behavior makes the most sense, I would expect the isLoading state to begin as true and remain true until the data is first retrieved.

image

SeanCassiere commented 2 weeks ago

Not sure what changed with the PeristQueryClientProvider, but as @RobertOstermann mentioned, when using the QueryClientProvider this works as expected.

@TkDodo any insights as to what's happening here?

Edit: Also, as @schiller-manuel mentioned for some reason throwing an empty loader into __root seems to fix it 😅. https://stackblitz.com/edit/github-fucdao-miae5w?file=src%2Fmain.tsx

TkDodo commented 2 weeks ago

Generally, restoring from any storage is an async process. So with the PersistQueryClientProvider, there is at least one render cycle where the Observer will be in the following state:

status: 'pending'
fetchStatus: 'idle'

for that combination of states, isLoading will be false. The app will still be rendered (mainly to not completely block SSR), but queries will not be subscribed yet.

Then, after the restore process is complete, the Query will be in whatever state it is supposed to be. If data is restored, and that data is fresh, it will be:

status: 'success'
fetchStatus: 'idle'

If the restored data is stale, you should see a background refetch:

status: 'success'
fetchStatus: 'fetching'

But in both of these cases, isLoading should be false, because that flag is only true for a "hard loading state", and once you restore data successfully from localstorage, you won't be "hard loading" ever again.

Interestingly, when I log those states inside PostsComponent:

console.log(postsQuery.status, postsQuery.fetchStatus);

I get the following output:

pending idle
pending fetching
Fetching posts...
pending fetching
pending fetching
success idle
success idle

pending idle is expected (like I explained before), but pending fetching is not. This should be success fetching or success idle. So it feels like the restore never really takes places here.

After debugging this further, I can see that you've set maxAge: 1000 on the persistOptions, meaning the data that's stored will be deemed invalid after ONE SECOND. That explains it, because every restore will now see data that is older than maxAge and will just discard it.

After removing the custom maxAge setting, I get the expected output:

pending idle
success fetching
Fetching posts...
success fetching
success idle

Can I get my 30 minutes back please ...

RobertOstermann commented 2 weeks ago

@TkDodo I think with the maxAge: 1000, while not a realistic real world example, still represents the scenario when query data is discarded due to the user not opening the site for 24 hours (default max age). The initial state does not have any data, but is in the process of restoring the data. To me that would indicate it is loading and it would be nice if the isLoading state reflected that, regardless of if the loading is retrieving data from local storage or actually querying for data.

I was using the isLoading state to determine if the query completed. Would it be better if I used isSuccess to determine this? Or would the useIsRestoring hook be something I should be using in addition to isLoading, knowing that when both are false the data has completed the restore/fetch process?

Also, I do realize that using loader and useSuspenseQuery avoids this problem, so I will be able to move my queries to loaders and be good to go, but I was confused as to the change in initial state and the affect of an empty loader, which is why I initially created this issue. If these loading states are intentional, you can close out this issue. And thanks to everyone for their time looking into this!

TkDodo commented 2 weeks ago

okay so the problem is in defining what isLoading means. It's really just: status === 'pending' && fetchStatus === 'fetching', meaning: "We have no data and we are currently fetching it for the first time".

When you restore from localstorage, it's not true because we have to wait for the data to be restored in order to know how old it is and if there will be a fetch. Then, it switches to true because the restored data will be discarded.

I was using the isLoading state to determine if the query completed.

Not sure what you mean by "query is completed". if (!isLoading) does not mean that data will be available - TypeScript also doesn't narrow here. if (data) or if (isSuccess) would be better checks for that.

RobertOstermann commented 2 weeks ago

Ok sounds good, seems like my understanding of how isLoading would work on the persist query provider was incorrect. Closing as it sounds like this is working as expected.