query icon indicating copy to clipboard operation
query copied to clipboard

[v4] new data on the first page will result in the last element disappearing from the list on revalidate*

Open myxoh opened this issue 3 years ago • 6 comments

Describe the bug

For starters - this is maybe a feature request / maybe a bug. It was certainly unexpected behaviour when starting using the library, but it makes sense looking at the internals.

This bug will results on: When an API orders from most recent, to last. Using fixed page size pagination. After a revalidate. If a new resource is created. an Element will be lost from the list even though it may still be present on the database / server.

Scenario: I have an API starting ordering from newest to most recent "0". GET /resources?pageSize=20 (Give records: 2021, 2021, 2020, 2019, ...etc) I'm using useInfiniteQuery starting from page 0. I compute the cursor for the next page using the last valid result: Cursor is computed as: 2001 GET /resources?pageSize=20&afterCursor=2001 this will give me: 2000, 1999, etc..., 1981

When a new entry is generated - the revalidation checks the first page first: GET /resources?pageSize=20 Which will now start on 2022, 2021, 2020... 2002 Next cursor is now 2002 instead of 2001 so the API will cut-off at 1982

If the user was looking at the record: 1981 - this will no longer be on the list.

This seems to be occurring because the revalidate process finishes at (what I think is arbitrarily) the same number of pages as what we had before.

It would be useful to have a configuration to control the process beyond just turning it on/off- allowing me to - say - ensure I stop at on, or after the first page including 1981..

Your minimal, reproducible example

https://codesandbox.io/s/strange-sea-j67vbs

Steps to reproduce

  1. Load the initial data set
  2. Load more (using the "load older" button)
  3. Click "Add newer content to store"
  4. Click away from the iframe (unfocus)
  5. Refocus.

Actual: The last element on the list will dissappear / is no longer visible

Expected behavior

I should still have the last element on the list

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

All

react-query version

3.39.2

TypeScript version

No response

Additional context

I believe all the necessary context was added.

myxoh avatar Sep 06 '22 11:09 myxoh

I think you didn't save the sandbox because there is no Add newer content to store button, but I think I get the problem.

I don't think that refetching should add new pages, and I also don't think that the cutoff is arbitrary. The problem is more around an api using fixed-sized pagination together with dynamic cursors. I am not sure how we would technically solve this problem in react-query. What would be your proposal on how to customize this?

TkDodo avatar Sep 06 '22 11:09 TkDodo

Hey Tk - thanks - sorry I've updated the link. The problem will also occur with numbered pagination provided the page size is fixed and the underlying data results are ordered newest => latest. I can create another example with number pagination if it helps demonstrate that.

myxoh avatar Sep 06 '22 11:09 myxoh

Some proposed fixes

  1. Allowing me to control the condition that decides when to stop fetching pages. For instance converting this for loop into a while loop - with a dynamic condition that defaults to the existing default.

Current: From: https://github.com/TanStack/query/blob/main/packages/query-core/src/infiniteQueryBehavior.ts:130

Proposed (idea) - rather than chaining promises in a for loop - you could have promises that fired up new promises internally based on whether you should fetch the next page. That way you could then check all the oldPages data and newPages data, as well as currentPage index on a user-configurable function that will determine the ending point. By default - that function would resolve when the currentPage index is the last index (to match the existing behaviour)

  1. Making the whole revalidate a user configurable option (granted - this is probably too much to require from consumer developers).

  2. Default to always keeping the last value on re-validation: This will require users to provide a comparison function that would determine if the results are before or after the last element. For example: hasReachedElement(page, oldLastElement): boolean - if configured - this would automatically keep revalidating until the function is true (in my example code- the hasReachedElement would compare the last element of the revalidated apge, with the numerical value of the oldLastElement with a >= indicator - and check the page length being empty.

I think this is would be awesome for me - but is likely too opinionated for this framework.


Alternatively, I totally understand if this doesn't seem like a problem you'd rather tackle in the library

myxoh avatar Sep 06 '22 11:09 myxoh

We already have something similar for imperative refetches like queryClient.refetchQueries. You can pass a refetchPage filter to decide if a certain page should be refetched or not:

// only refetch the first page
queryClient.refetchQueries({
  refetchPage: (_page, index) => index === 0,
})

We don't have this for automatically triggered refetches like window focus because it would be difficult to customize (it would need to go on useInfiniteQuery or something).

But again, the hard cutoff here is the current amount of pages. Adding another, similar way to determine when refetching should stop seems kinda complicated and we'd have two ways to configure this then?

However, I'm not totally in love with the current api. refetchQueries et al have this refetchPage filter, but it only works on infinite queries, which is not nice, api wise. Also, as stated, it doesn't work for window focus refetching.

So long story short: If we can come up with an api that we can maybe pass as new option to useInfiniteQuery that would:

  • keep the same default behaviour (refetch all currently existing pages)
  • still allow me to easily say things like "only fetch the first page"
  • allows for better control of when to stop refetching, potentially based on previous results so that we could also fetch more pages than we previously had

we could think about adding this to the next major in favour of the refetchPage filter option. Thoughts?

TkDodo avatar Sep 18 '22 20:09 TkDodo

@myxoh what do you think about my proposal? If you'd want something like that, can you provide ideas on how the api could look? We are currently thinking about the next major version and if we should do something in this regard - now would be the time to discuss it. Otherwise, I'll likely close this issue. Thanks.

TkDodo avatar Oct 29 '22 17:10 TkDodo

@myxoh we've started working on v5, so if we'd want to change something api wise - now would be the time.

TkDodo avatar Dec 25 '22 15:12 TkDodo

Hi @TkDodo appreciate - not sure if I should close but I've sort of dropped cognitive load from this given we've ended up going with a different solution.

I do think a new option for useInfiniteQuery that allows for better control seems like a good way of solving this particular problem!

I'm sorry I've not been of much help here

myxoh avatar Apr 18 '23 12:04 myxoh

yeah, not sure either. we've removed refetchPage in v5 for a maxPage option. Maybe this can help in this case as well.

TkDodo avatar Apr 19 '23 06:04 TkDodo