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

Relay pagination omits pageInfo fields #2521

Open
r0busta opened this issue Jun 25, 2022 · 3 comments
Open

Relay pagination omits pageInfo fields #2521

r0busta opened this issue Jun 25, 2022 · 3 comments
Labels
bug 🐛 Oh no! A bug or unintented behaviour. help wanted ⛏️ Extra attention is needed

Comments

@r0busta
Copy link

r0busta commented Jun 25, 2022

For some reason, the relayPagination implementation doesn't copy the response pageInfo but instead picks only specific fields.

This behavior doesn't seem correct as it does not conform to the Relay spec. The spec says that the server MAY return hasPreviousPage: true for the forward pagination and hasNextPage: true for the backward one. However, the urql implementation assumes that hasPreviousPage for the forward pagination and hasNextPage, for the backward one, respectively, response fields can be ignored.

I'm happy to contribute a fix for this.

urql version & exchanges:

  • urql@^2.2.2
  • @urql/exchange-graphcache@^4.4.3

Expected behavior

relayPagination() returns all pageInfo fields regardless of to query variables.

Actual behavior

relayPagination() returns only:

  • endCursor and hasNextPage when query vars have after
  • startCursor and hasPreviousPage when query vars have before
@r0busta r0busta added the bug 🐛 Oh no! A bug or unintented behaviour. label Jun 25, 2022
@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Jun 26, 2022

As far as I can tell we always include all the fields we are getting from your query relevant code are you adding all the fields in your selection-set? We have a test here showing that it should work 😅

@r0busta
Copy link
Author

r0busta commented Jun 27, 2022

Yes, I include all fields in the query.

The test that you pointed out obviously passes because it checks the default case when the query requests the first page (the code here). However, I'm talking about cases when the query paginates using after or before.

If you change this test changed as following, it will fail:


it('works with forward pagination', () => {
  const Pagination = gql`
    query($cursor: String) {
      __typename
      items(first: 1, after: $cursor) {
        __typename
        edges {
          __typename
          node {
            __typename
            id
          }
        }
        nodes {
          __typename
          id
        }
        pageInfo {
          __typename
          hasNextPage
          endCursor
          hasPreviousPage
          startCursor
        }
      }
    }
  `;

  const store = new Store({
    resolvers: {
      Query: {
        items: relayPagination(),
      },
    },
  });

  const pageOne = {
    __typename: 'Query',
    items: {
      __typename: 'ItemsConnection',
      edges: [itemEdge(1)],
      nodes: [itemNode(1)],
      pageInfo: {
        __typename: 'PageInfo',
        hasNextPage: true,
        endCursor: '1',
      },
    },
  };

  const pageTwo = {
    __typename: 'Query',
    items: {
      __typename: 'ItemsConnection',
      edges: [itemEdge(2)],
      nodes: [itemNode(2)],
      pageInfo: {
        __typename: 'PageInfo',
        hasNextPage: false,
        endCursor: null,
        hasPreviousPage: true,
      },
    },
  };

  write(store, { query: Pagination, variables: { cursor: null } }, pageOne);
  write(store, { query: Pagination, variables: { cursor: '1' } }, pageTwo);

  const res = query(store, { query: Pagination });

  expect(res.partial).toBe(false);
  expect(res.data).toEqual({
    ...pageTwo,
    items: {
      ...pageTwo.items,
      edges: [pageOne.items.edges[0], pageTwo.items.edges[0]],
      nodes: [pageOne.items.nodes[0], pageTwo.items.nodes[0]],
    },
  });
});

BTW, that test checks if endCursor for page two is null, while both endCursor and startCursor should be 2. Because page two has nodes, even though only one, and its cursor is 2. However, the test passes for both null and non-null cursors because the actual and expected values for the expect are build from the same source.

Essentially, @urql/exchange-graphcache@<=4.4.3 doesn't support paginating forward and then paginating backward.

@BahaaZidan
Copy link

We're running into the same issue. I'm positive that I'm querying pageInfo.hasPreviousPage and also positive that my server is returning the correct value for it. But the URQL client is always returning it as false.

Is someone working on a fix for this ? I'm happy to contribute.

@kitten kitten added the help wanted ⛏️ Extra attention is needed label Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Oh no! A bug or unintented behaviour. help wanted ⛏️ Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants