urql icon indicating copy to clipboard operation
urql copied to clipboard

Relay pagination omits pageInfo fields

Open r0busta opened this issue 3 years ago • 3 comments

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 avatar Jun 25 '22 19:06 r0busta

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 😅

JoviDeCroock avatar Jun 26 '22 14:06 JoviDeCroock

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.

r0busta avatar Jun 27 '22 08:06 r0busta

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.

BahaaZidan avatar Nov 03 '22 14:11 BahaaZidan