relayStylePagination returns too many results

[sleepycat]

I'm trying to write a test that paginates using the relayStylePagination function and seeing behaviour I don't understand.

Basically imagine starting with the markup below, showing "repo one", clicking next, seeing "repo two" and then clicking previous which should display "repo one" again.

      <div>
        <ul>
          <li>
            repo one
          </li>
        </ul>
        <button>
          next
        </button>
        <button>
          previous
        </button>
      </div>

Clicking next shows both "repo two" (what I was expecting) and "repo one" (which I wasn't):

      <div>
        <ul>
          <li>
            repo one
          </li>
          <li>
            repo two
          </li>
        </ul>
        <button>
          next
        </button>
        <button>
          previous
        </button>
      </div>

If I click previous "repo one" is duplicated, and "repo two" is still visible:

      <div>
        <ul>
          <li>
            repo one
          </li>
          <li>
            repo one
          </li>
          <li>
            repo two
          </li>
        </ul>
        <button>
          next
        </button>
        <button>
          previous
        </button>
      </div>

I made a codesandbox demonstrating the issue with the current version (3.1.3).

[benjamn]

For your backward query, you're using the variables { before: "Y3Vyc29yOnYyOpHOAAfgfQ==", last: 1 }, where the before cursor identifies the repo one result, but then the result you're returning for that fetch includes the repo one edge again (with the same cursor). Shouldn't the backward result either return no edges (if there's nothing before the first result), or a different edge (maybe edge zero), with its own distinct cursor? Given that you return edge one again, I think it's pretty reasonable for the client to assume you really meant to add another repo one edge before the original repo one edge, even though you did not intend to duplicate that edge.

[sleepycat]

Sorry, that was supposed to paginate back to repo one again.
I've updated the fetchMore for the BACKWARD query to the correct cursor: { before: "Y3Vyc29yOnYyOpHOAA_zRw==", last: 1 }, which points back to the original repo one edge.

With that fixed, the idea is that looking at "repo one", and clicking next should display the result for repo two and only that result. The test shows that both repo one and repo two are displayed.

      <div>
        <ul>
          <li>
            repo one
          </li>
          <li>
            repo two
          </li>
        </ul>
        <button>
          next
        </button>
        <button>
          previous
        </button>
      </div>

If you remove the expect(queryByText(/repo one/)).not.toBeInTheDocument(); on line 227 instead of displaying both results on page two, it displays both results after returning to page one. This is what I'm getting at with the "too many results" thing.

As soon as next is clicked, both results are in the cache, and both are being displayed.
Since I'm only asking for one, I should only get one. Looking at the implementation I was expecting to see the first/last values being used somewhere with a slice, but maybe that happens elsewhere.

Thanks for looking at this.

[benjamn]

Ok, thanks for the update!

With that out of the way, I think the next thing to explain is that the relayStylePagination helper (like the other helpers in that module) chooses not to implement a custom read function to match the merge function, which means the original query will always receive the full known list, which can be sliced/paginated/filtered/etc. in application code. This matches the way fetchMore has traditionally worked, though I have to admit it was a surprise to me that you might not need a read function.

Here's another comment where I get into these details: #6729 (comment). Specifically this bit:

As a final note, it may be tempting to define a read function to go along with your merge function, but we've found that read functions are often unnecessary when using fetchMore, and can complicate things because you have to update the variables for the original query to match the new longer list. With no read function and keyArgs: false, the list just keeps growing, and you don't have to worry about updating the original query.

I dug up an older (untested!) implementation of relayStylePagination that did have a read function, if you're curious what that might look like. If you want to use that code, ignore the merge function (it's old and definitely has bugs) and just borrow the read function, and make sure you update the variables of the original query (maybe in a .then callback after fetchMore) to match the list you want to slice out. This has been a very tricky bit of code to get right, even with a lot of tests, so I can't vouch for every detail of this old code. Don't hesitate to blame me if it doesn't work!

[jdpst]

@benjamn Is there any plan for the relayStylePagination helper to handle the first and last Relay parameters? It is very surprising that it does not already do so IMO.

To make sure I am understanding, could you clarify for me that:

1. this helper always returns the full list of cached results, not the requested results using first/last/before/after (i.e. does not support actual pagination as is)

2. fixing the above bug requires a custom read function (and therefore also maintaining the duplicated merge function)

3. writing robust custom read functions requires specialist knowledge of how the cache works (e.g. incorporating keyArgs) and is error-prone

I am hoping this can be resolved in a patch sometime soon, because the updateQuery API (which works without fuss) is being deprecated.

---

Update: I have taken a stab at rewriting the helper (below), which works as I would have expected the original to. Would be interested to see your thoughts on it. There are several problems I had with the original, which I believe are solved:

1. the original relied on the arguments being passed directly to the field, i.e. myField(first: Int, ...) whereas this is agnostic to your arguments; it is concerned only with your results. We were wrapping the arguments into an object, like myField(paginationOptions) so would have had to rewrite a lot of code without this; this offers ability to have more flexibility in the schema without losing functionality.

2. the original did not try to update the cache when the function was called with no arguments, now it does the same checks it would normally do to find the prefix/suffix and merges the edges accordingly.

3. the original modified the pageInfo returned by the backend, this does not. I couldn't think of any reason I would want it to be modified; on the contrary, modifying pageInfo makes reading the desired results more difficult.

4. this supports both "infinite-scroll" type behaviour that exists currently, and also pagination.

Usage looks like:

...,
typePolicies: {
    Query: {
        fields: {
            users: relayStylePagination(keyArgs, {usePagination: true}),
        },
    },
},
...

type TInternalRelay<TNode> = Readonly<{
  edges: Array<{
    cursor: string;
    node: TNode;
  }>;
  pageInfo: Readonly<{
    hasPreviousPage: boolean;
    hasNextPage: boolean;
    startCursor?: string;
    endCursor?: string;
  }>;
}>;

interface RelayStylePaginationOptions {
  usePagination?: boolean;
}

// As proof of the flexibility of field policies, this function generates
// one that handles Relay-style pagination, without Apollo Client knowing
// anything about connections, edges, cursors, or pageInfo objects.
export function relayStylePagination<TNode = Reference>(
  keyArgs: KeyArgs = false,
  options?: RelayStylePaginationOptions,
): FieldPolicy<TInternalRelay<TNode>> {
  return {
    keyArgs,

    read(existing, {canRead}) {
      if (!existing) return;

      let edges = existing.edges.filter((edge) => canRead(edge.node));

      // Note I have left the exisiting "infinite-scroll" behaviour as the default to
      // avoid any breaking changes, although I strongly believe that "normal" pagination
      // ought to be.
      if (options?.usePagination) {
        const pageInfo = existing.pageInfo;
        const startIndex = edges.findIndex((edge) => edge.cursor === pageInfo?.startCursor);
        if (startIndex > -1) edges = edges.slice(startIndex);
        const endIndex = edges.findIndex((edge) => edge.cursor === pageInfo?.endCursor);
        if (endIndex > -1) edges = edges.slice(0, endIndex + 1);
      }

      return {
        // Some implementations return additional Connection fields, such
        // as existing.totalCount. These fields are saved by the merge
        // function, so the read function should also preserve them.
        ...existing,
        edges,
      };
    },

    merge(existing = makeEmptyData<TNode>(), incoming) {
      const startCursor = incoming?.pageInfo?.startCursor;
      const endCursor = incoming?.pageInfo?.endCursor;

      let prefix: typeof existing.edges = [];
      let suffix: typeof existing.edges = [];

      // Find prefix
      if (startCursor) {
        const index = existing.edges.findIndex((edge) => edge.cursor === startCursor);
        prefix = index > -1 ? existing.edges.slice(0, index) : existing.edges;
      }

      // Find suffix
      if (endCursor) {
        const index = existing.edges.findIndex((edge) => edge.cursor === endCursor);
        suffix = index > -1 ? existing.edges.slice(index + 1) : [];
      }

      const edges = [...prefix, ...incoming.edges, ...suffix];

      // Modify the edges ONLY.
      return {
        ...incoming,
        edges,
      };
    },
  };
}

function makeEmptyData<TNode>(): TInternalRelay<TNode> {
  return {
    edges: [],
    pageInfo: {
      hasPreviousPage: false,
      hasNextPage: false,
    },
  };
}

[raysuelzer]

This API change seems to make a pretty big assumption that developers won't be using traditional pagination and infinite scroll for the same query.

[hwillson]

Let us know if this is still a concern with @apollo/client@latest - thanks!