Infinite Queries: wrong auto refetching with manual pageParams

[TkDodo]

Describe the bug

Infinite Queries can be used in two ways:

• they implement getNextPageParam / getPreviousPageParam (automatic)
• they pass pageParam directly to fetchNextPage / fetchPreviousPage (manual)

Mixing those two leads to a weird behaviour.

Your minimal, reproducible example

https://codesandbox.io/s/dazzling-lehmann-zibq9m?file=/pages/index.js

Steps to reproduce

1. scroll down, click on load newer
2. now, click the skip button
3. you now have projects 0-9 and 50-54 on screen. The stored pageParams are:
   [0, 5, 50]
4. trigger an automatic refetch by re-focussing the window

--> you now have projects 0-14 on screen
pageParmas are:
[0, 5, 10]

Expected behavior

data on screen should not change / stay as it was initially fetched, and pageParams shouldn't change.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

all

react-query version

4

TypeScript version

No response

Additional context

The problem seems to be that during an automatic refetch, getNextParam is always invoked when it exists. We have no knowledge of the fact that the 3rd page was fetched with the "manual" approach.

It seems that mixing both approaches is not supported at all, even though there is an extra section in the docs that does exactly that:

https://tanstack.com/query/v4/docs/guides/infinite-queries#what-if-i-need-to-pass-custom-information-to-my-query-function

when we refetch pages, we determine if we are in manual mode or not by the presence of the getNextPageParam function:

query/packages/query-core/src/infiniteQueryBehavior.ts

Line 122 in d5c4330

const manual = typeof context.options.getNextPageParam === 'undefined'

But that function will always be present, even when a single page fetch was "overwritten" with a manual fetch.

---

One way to fix this would be to extend pageParams (or the whole infinite query data structure) by the information if the fetch happened initially as manual or not.

Actually, we only store pageParams for refetches when everything is using the manual approach. Put another way: We never access pageParams if a getNextPageParam function is present.

[prateek3255]

One way to fix this would be to extend pageParams (or the whole infinite query data structure) by the information if the fetch happened initially as manual or not.

@TkDodo Wouldn't that be a breaking change because pageParams is stored as an array right now

[TkDodo]

@TkDodo Wouldn't that be a breaking change because pageParams is stored as an array right now

yes, could be something for v5. Or, we extend the structure to be like:

pages: [{..}, {..}, {..}],
pageParams: [undefined, 5, 50],
manual: [false, false, true]

but this seems overly verbose and unnecessary.

One question is: Why even store pageParams if we didn't fetch manually? I think we could also just store pageParams IF we fetched manually and use undefined for other cases, so in the above example:

pages: [{..}, {..}, {..}],
pageParams: [undefined, undefined, 50],

if we encounter undefined, we would just invoke getNextPageParam (except for the first page, which always needs undefined passed), and if we have a stored value, we would use that.

[TkDodo]

Just tried this approach out and it doesn't work if you use bi-directional queries. Like, fetch first page, then fetchNext, then fetchPrevious. You would have 3 pages and all of them would have pageParam: undefined. The refetch then starts at the first entry in the array, which is the "previous page", so we'd need that value's pageParam from the fetch as well.

So my statement that we "only use the stored pageParams when we did manual fetching" was wrong ...

This seems to be pretty hard to fix conceptually 😅

[prateek3255]

@TkDodo Okay then how about we go with your initial idea of using an array of objects, with the manual key and keep the pageParams as well for backwards compatibility. Something like this:

pages: [{..}, {..}, {..}],
pageParams: [undefined, 5, 50],
pageParamsWithManual: [{ page: undefined, manual: false }, { page: 5, manual: false }, { page: 50, manual: true }]

Internally we use the pageParamsWithManual (can be named something better) and rely on the manual key when refetching. With v5 deprecate the array format and use the new format for pageParams. This should do it right?

[TkDodo]

I think even extending the object with another entry could break something, for example, if you implemented optimistic updates without spreading:

queryClient.setQueryData(
  key,
  (prev) => prev ? { pages: update(prev.pages), pageParams: prev.pageParams } : prev
)

this is legit code in v4, as it returns an object with pages and pageParams, but it would take away pageParamsWithManual.

I think we should wait with this change for v5 and do your proposed change there.

[prateek3255]

Hey @TkDodo Since we have started working on v5 can I take up this issue and fix it for v5?

[TkDodo]

I'll write up a dedicated RFC for infinite queries in v5 soon. It will impact this issue so let's please hold off for now

[TkDodo]

PR:

• Feature/infinite queries #5004