Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v3]: bidirectional useInfiniteQuery breaks pages order on refetch #1401

Closed
monstasat opened this issue Dec 14, 2020 · 9 comments
Closed

[v3]: bidirectional useInfiniteQuery breaks pages order on refetch #1401

monstasat opened this issue Dec 14, 2020 · 9 comments

Comments

@monstasat
Copy link

monstasat commented Dec 14, 2020

Describe the bug
Bidirectional infinite query doesn't consider direction when refetching.
According to the code, getNextPageParam is called for each page while updating a query, no matter how it was originally fetched - by calling fetchNextPage or fetchPreviousPage.

Refetch works fine for unidirectional queries. For bidirectional queries refetch routine just loads the same amount of pages as was previously loaded, but in next direction only.

So, if the original request order was:

  1. initial fetch
  2. fetch next
  3. fetch previous
    ...

After refetch it will be:

  1. initial fetch
  2. fetch next
  3. fetch next
    ...

Expected behavior
When refetching an infinite query, original direction in which pages were loaded should be considered. As long as backend responds with the same data for the same page params, refetching a query should not break page data order or structure.

Additional context
v 3.2.0

@boschni
Copy link
Collaborator

boschni commented Dec 14, 2020

I think it should currently work like this:

Initial:

  1. initial fetch (page 5)
  2. fetch next (page 6)
  3. fetch previous (page 4)

Pages: [4, 5, 6]

Refetch:

  1. refetch (page 4)
  2. fetch next (page 5)
  3. fetch next (page 6)

Pages: [4, 5, 6]

Is this behavior causing issues in your case?

@monstasat
Copy link
Author

Yes, I think there is an issue regarding getPreviousPageParam and getNextPageParam functions may return different values. In my particular case, I return an object containing direction: 'asc' when loading next data and an object with direction: 'desc' for the previous data. Current behaviour breaks this logic by calling only getNextPageParam on refetch, causing only requests with direction: 'asc' to be produced.

@boschni
Copy link
Collaborator

boschni commented Dec 14, 2020

Think usually only the cursor changes between fetches while keeping filters and sorting the same. Do you maintain some kind of session state on the server?

@monstasat
Copy link
Author

monstasat commented Dec 14, 2020

No, the server is stateless. The API design I'm trying to implement is as follows:

  • each server response has two cursors: ascending and descending. We use ascending cursor to fetch data from the tail of the list, and descending cursor to fetch from the head. So, the response looks like
{ 
  "cursor_asc": "foo",
  "cursor_desc: "bar",
  "data": []
}
  • when making a request, both cursors should be sent to the server alongside with direction field, which can take asc and desc values. The server knows which cursor to use with the help of this direction field. For example, if direction is desc, the server responds with data to be prepended to the head of the list. In this case, cursor_desc is updated in server response, and cursor_asc remains the same. So, the request looks like:
{
  "cursor_asc": "foo",
  "cursor_desc: "bar",
  "direction": "desc"
}

And the corresponding response may be like:

{
  "cursor_asc": "foo",
  "cursor_desc": "some_new_cursor",  // Note only this cursor has changed
  "data": []
}

As you can see below, current useInfiniteQuery behaviour breaks this logic. Here is a dummy example:


Imagine we have the following DB records:

Records
1
2
3
4
5

Step 1

I do the initial request and receive

{
 data: [4,5],
 cursor_desc: "foo" // points to the value 4
 cursor_asc: "bar",  // points to the value 5
 }

getPreviousPageParams will return { direction: 'desc', cursor_asc: 'foo', cursor_desc: 'bar' } here.
getNextPageParams will return { direction: 'asc', cursor_asc: 'foo', cursor_desc: 'bar' }.

Step 2

I scroll to the top of the list and call fetchPreviousData from useInfiniteQuery and receive

{ 
  data: [2, 3],
  cursor_desc: "new_foo" // changed, now points to the value 2
  cursor_asc: "bar", // not changed, points to the value 5
}

Here I have two pages of data. Now it's time to refetch the results. With current implementation of useInfiniteQuery, the following requests would be made:

  • initial request: will return { data: [4,5], cursor_asc: "foo", cursor_desc "bar" }. It is the same as before
  • next request: will return { data: [], cursor_asc: "foo", cursor_desc: "bar" } (because it was sent with direction: 'asc' and there is no data available yet after in this direction - 5 is the last value in DB). It is completely different from the original data we have received on step 2.

Since I have different cursors pointing to the head and the tail of the list, I expect requests to be made in the same order and with same parameters while refetching as they were originally sent.

Sorry for being a bit verbose, just trying to explain the issue better :D

@boschni
Copy link
Collaborator

boschni commented Dec 15, 2020

Thanks for the additional explanation, the more information the better :)

On refetch React Query should start with the first page in the list (resulting into [2, 3]), and then feed the result into getNextPageParams to get the next page and so on.

This response does seem a bit strange to me:

{ 
  data: [2, 3],
  cursor_desc: "new_foo" // changed, now points to the value 2
  cursor_asc: "bar", // not changed, points to the value 5 <-- shouldn't this point to 3?
}

@monstasat
Copy link
Author

On refetch React Query should start with the first page in the list (resulting into [2, 3]), and then feed the result into getNextPageParams to get the next page and so on.

I don't fully understand this logic. I agree that API I have mentioned is not very consistent, but we may switch to another example.

Here is the JSON API doc about cursor pagination. As I can see, we face the similar issue here.
If we have loaded the initial page and then some pages with page[before] cursor, after refetching the query we will have completely different data in the cache. This is because instead of sending page[before] cursor to the server we will send page[after].

@boschni
Copy link
Collaborator

boschni commented Dec 15, 2020

A flow with JSON API would probably look something like this?

Data:

[
  { "type": "examples", "id": "1" },
  { "type": "examples", "id": "5" },
  { "type": "examples", "id": "7" },
  { "type": "examples", "id": "8" },
  { "type": "examples", "id": "9" }
]

Fetch initial:

// Request
{}

// Response
{
  "data": [
    { "type": "examples", "id": "7", "meta": { "page": { "cursor": "cursor7" } } },
    { "type": "examples", "id": "8", "meta": { "page": { "cursor": "cursor8" } } }
  ]
}

Fetch previous:

// Request
getPreviousPageParams(firstPage)
{ "page[before]": "cursor7" }

// Response
{
  "data": [
    { "type": "examples", "id": "1", "meta": { "page": { "cursor": "cursor1" } } },
    { "type": "examples", "id": "5", "meta": { "page": { "cursor": "cursor5" } } }
  ]
}

Result:

[
  {
    "data": [
      { "type": "examples", "id": "1", "meta": { "page": { "cursor": "cursor1" } } },
      { "type": "examples", "id": "5", "meta": { "page": { "cursor": "cursor5" } } }
    ]
  },
  {
    "data": [
      { "type": "examples", "id": "7", "meta": { "page": { "cursor": "cursor7" } } },
      { "type": "examples", "id": "8", "meta": { "page": { "cursor": "cursor8" } } }
    ]
  }
]

Refetch request 1:

// Request
{ "page[before]": "cursor7" }

// Response
{
  "data": [
    { "type": "examples", "id": "1", "meta": { "page": { "cursor": "cursor1" } } },
    { "type": "examples", "id": "5", "meta": { "page": { "cursor": "cursor5" } } }
  ]
}

Refetch request 2:

// Request
getNextPageParams(lastPage)
{ "page[after]": "cursor5" }

// Response
{
  "data": [
    { "type": "examples", "id": "7", "meta": { "page": { "cursor": "cursor7" } } },
    { "type": "examples", "id": "8", "meta": { "page": { "cursor": "cursor8" } } }
  ]
}

Result:

[
  {
    "data": [
      { "type": "examples", "id": "1", "meta": { "page": { "cursor": "cursor1" } } },
      { "type": "examples", "id": "5", "meta": { "page": { "cursor": "cursor5" } } }
    ]
  },
  {
    "data": [
      { "type": "examples", "id": "7", "meta": { "page": { "cursor": "cursor7" } } },
      { "type": "examples", "id": "8", "meta": { "page": { "cursor": "cursor8" } } }
    ]
  }
]

@monstasat
Copy link
Author

Thank you for the detailed reply. The problem is that I've missed that refetching starts from the first page in the pages array, not from the initial request.

I will make sure that this is working for my case and then return with feedback ☺️

@boschni
Copy link
Collaborator

boschni commented Dec 16, 2020

No problem! Let me know if you are encountering any problems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants