From edb5c38b8cfaf873b7b02a8d9d899dba195b4200 Mon Sep 17 00:00:00 2001 From: Sam Magura Date: Thu, 9 Jun 2022 14:07:16 -0400 Subject: [PATCH] Change behavior of relayStylePagination merge function It now replaces existing edges when it receives an unknown cursor. --- .../__tests__/relayStylePagination.test.ts | 142 ++++++++++++++++++ src/utilities/policies/pagination.ts | 49 ++++-- 2 files changed, 179 insertions(+), 12 deletions(-) diff --git a/src/utilities/policies/__tests__/relayStylePagination.test.ts b/src/utilities/policies/__tests__/relayStylePagination.test.ts index a2e19321bd4..02fd8c80d6f 100644 --- a/src/utilities/policies/__tests__/relayStylePagination.test.ts +++ b/src/utilities/policies/__tests__/relayStylePagination.test.ts @@ -317,5 +317,147 @@ describe('relayStylePagination', () => { } }); }); + + it("replaces the existing data if a query using `after` is repeated", () => { + const page = { + edges: [{ cursor: "alpha", node: makeReference("fakeAlpha") }], + pageInfo: { + hasPreviousPage: false, + hasNextPage: true, + startCursor: "alpha", + endCursor: "alpha", + }, + }; + + const result = merge(page, page, { + ...options, + args: { + // Represents the cursor that comes immediately before 'alpha' + after: "zeta", + }, + }); + + expect(result).toEqual(page); + }); + + it("replaces the existing data if a query using `before` is repeated", () => { + const page = { + edges: [{ cursor: "alpha", node: makeReference("fakeAlpha") }], + pageInfo: { + hasPreviousPage: true, + hasNextPage: false, + startCursor: "alpha", + endCursor: "alpha", + }, + }; + + const result = merge(page, page, { + ...options, + args: { + // Represents the cursor that comes immediately after 'alpha' + before: "beta", + }, + }); + + expect(result).toEqual(page); + }); + + it("slices the existing data if two pages overlap when using `after`", () => { + const existingEdges = [ + { cursor: "alpha", node: makeReference("fakeAlpha") }, + { cursor: "beta", node: makeReference("fakeBeta") }, + ]; + + const incomingEdges = [ + { cursor: "beta", node: makeReference("fakeBeta2") }, + { cursor: "gamma", node: makeReference("fakeGamma") }, + ]; + + const result = merge( + { + edges: existingEdges, + pageInfo: { + hasPreviousPage: false, + hasNextPage: true, + startCursor: "alpha", + endCursor: "beta", + }, + }, + { + edges: incomingEdges, + pageInfo: { + hasPreviousPage: true, + hasNextPage: true, + startCursor: "beta", + endCursor: "gamma", + }, + }, + { + ...options, + args: { + after: "alpha", + }, + } + ); + + expect(result).toEqual({ + edges: [existingEdges[0], ...incomingEdges], + pageInfo: { + hasPreviousPage: false, + hasNextPage: true, + startCursor: "alpha", + endCursor: "gamma", + }, + }); + }); + + it("slices the existing data if two pages overlap when using `before`", () => { + const existingEdges = [ + { cursor: "beta", node: makeReference("fakeBeta") }, + { cursor: "gamma", node: makeReference("fakeGamma") }, + ]; + + const incomingEdges = [ + { cursor: "alpha", node: makeReference("fakeAlpha") }, + { cursor: "beta", node: makeReference("fakeBeta2") }, + ]; + + const result = merge( + { + edges: existingEdges, + pageInfo: { + hasPreviousPage: true, + hasNextPage: false, + startCursor: "beta", + endCursor: "gamma", + }, + }, + { + edges: incomingEdges, + pageInfo: { + hasPreviousPage: true, + hasNextPage: true, + startCursor: "alpha", + endCursor: "beta", + }, + }, + { + ...options, + args: { + before: "gamma", + }, + } + ); + + expect(result).toEqual({ + edges: [...incomingEdges, existingEdges[1]], + pageInfo: { + hasPreviousPage: true, + hasNextPage: false, + startCursor: "alpha", + endCursor: "gamma", + }, + }); + }); }) }); diff --git a/src/utilities/policies/pagination.ts b/src/utilities/policies/pagination.ts index d331f097e34..c3330e68913 100644 --- a/src/utilities/policies/pagination.ts +++ b/src/utilities/policies/pagination.ts @@ -89,9 +89,33 @@ export type RelayFieldPolicy = FieldPolicy< TIncomingRelay | null >; -// 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. +// 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. + +// There is one case where the `merge` function does not have enough information +// to do the optimal correctly in all cases. +// +// Say we have one page in the cache and are doing `after`/`first` pagination. +// Then, the `merge` function receives an incoming page and the `after` used to +// request that page does not match any of the existing edges' cursors. +// +// 1. If the new `after` is the same as the original `after`, i.e. the incoming +// page is the result of repeating the original query, the incoming edges +// should REPLACE the existing edges. +// +// 2. If the new `after` comes from later in the sequence of cursors, i.e. there +// is a gap between the existing page and the incoming page, the incoming +// edges should probably be APPENDED to the existing edges. Though, the +// expected behavior could depend on what the consuming application is trying +// to accomplish. +// +// The `merge` function cannot distinguish between (1) and (2), so it cannot do +// the correct thing in both cases. (1) is likely to happen in real +// applications, while (2) would only happen if there is a bug in the consuming +// application or the consuming application has a non-standard use case. Because +// (1) is the more important case to handle correctly, the `merge` function +// below is coded to REPLACE the existing edges with the incoming edeges. export function relayStylePagination( keyArgs: KeyArgs = false, ): RelayFieldPolicy { @@ -188,27 +212,28 @@ export function relayStylePagination( } } - let prefix = existing.edges; - let suffix: typeof prefix = []; + let prefix: TRelayEdge[] = []; + let suffix: TRelayEdge[] = []; if (args && args.after) { // This comparison does not need to use readField("cursor", edge), // because we stored the cursor field of any Reference edges as an // extra property of the Reference object. - const index = prefix.findIndex(edge => edge.cursor === args.after); + const index = existing.edges.findIndex((edge) => edge.cursor === args.after); if (index >= 0) { - prefix = prefix.slice(0, index + 1); - // suffix = []; // already true + prefix = existing.edges.slice(0, index + 1); } } else if (args && args.before) { - const index = prefix.findIndex(edge => edge.cursor === args.before); - suffix = index < 0 ? prefix : prefix.slice(index); - prefix = []; + const index = existing.edges.findIndex((edge) => edge.cursor === args.before); + if (index >= 0) { + suffix = existing.edges.slice(index); + } } else if (incoming.edges) { // If we have neither args.after nor args.before, the incoming // edges cannot be spliced into the existing edges, so they must // replace the existing edges. See #6592 for a motivating example. - prefix = []; + } else { + prefix = existing.edges; } const edges = [