relayStyleConnection: Cannot read property 'edges' of undefined in all versions >= v3.3.0

[SirensOfTitan]

Intended outcome:
After upgrading from 3.2.x to 3.3.x, pages load correctly without error.

Actual outcome:
We've been stuck on 3.2.x for the last several months due to this error when loading pages that use connections with relayStyleConnection-based type policies:

TypeError: Cannot read property 'edges' of undefined
    at merge (webpack-internal:///./node_modules/@apollo/client/utilities/utilities.cjs.js:704:45)
    at Policies.runMergeFunction (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:1299:20)
    at StoreWriter.applyMerges (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:880:40)
    at eval (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:863:34)
    at Map.forEach (<anonymous>)
    at StoreWriter.applyMerges (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:860:27)
    at StoreWriter.processSelectionSet (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:812:39)
    at StoreWriter.processFieldValue (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:839:21)
    at eval (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:782:47)
    at Set.forEach (<anonymous>)
    at StoreWriter.processSelectionSet (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:767:17)
    at StoreWriter.processFieldValue (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:839:21)
    at eval (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:782:47)
    at Set.forEach (<anonymous>)
    at StoreWriter.processSelectionSet (webpack-internal:///./node_modules/@apollo/client/cache/cache.cjs.js:767:17)

These errors, I think, are related to merging of connections with relayStyleConnection type policies. existing will look fine, something like:

{
  __typename: 'FooConnection',
  edges: [
    {
      __typename: 'FooEdge',
      cursor: 'MzkzNjc2M45DE3ODYw',
      node: [Object]
    }
  ],
  pageInfo: {
    __typename: 'PageInfo',
    startCursor: 'MzkzNjc45DE3ODYw',
    endCursor: 'MzkzADMjI4MjE3Yw',
    hasNextPage: false,
    hasPreviousPage: false
  }
}

And incoming will be undefined (and of course then incoming.edges is not defined).

How to reproduce the issue:
I cannot get a perfect isolated reproduction here, but I did bisect to a commit:
8172159

These errors only throw with relayStyleConnection. The point in the code this throws at is:
https://github.com/apollographql/apollo-client/blob/main/src/utilities/policies/pagination.ts#L130

Versions
Every version after and including 3.3.0-beta.4.

[benjamn]

@SirensOfTitan First of all, thanks for surfacing this issue instead of continuing to suffer in silence, and thanks for going to the trouble of bisecting it.

Do you have any other type or field policies defined where the relayStylePagination() policy is used? For example, are there any merge: true configurations for the field that holds this FooConnection object, or any places where you use the mergeObjects helper function in a field merge function?

It's very strange to me that incoming should ever be undefined, so if we can figure out why that happens, that's likely to be the/a bug.

[SirensOfTitan]

Of course! Thanks for creating an awesome library, sorry I couldn't come up with a proper reproduction!

We use type policies for mutations, connections, and subscriptions (none of the subscriptions interact with the connections that have this issue), no other query fields, and no use of merge: true or mergeObjects. Just to be sure, I'll remove the other typePolicies later today just to ensure they aren't the cause here.

[lukel97]

We are seeing this too whenever we have a nullable connection that returns null. Presumably when a null connection object is merged, the resulting object should be null too

[migueloller]

I'm seeing this issue and incoming seems to be undefined in the same situations where data is undefined even though error is undefined and loading is false. We actually had to wrap relayStylePagination so that we check if incoming is undefined and just return options.mergeObjects(existing, incoming) instead. It's very hard to pinpoint exactly what's causing this to happen but in the past we've seen it pop up when adding or removing fields or spreading certain fragments. I'm sorry I don't have a better explanation of what we've seen, but this is all we have to deal with at the moment, which is why we just wrapped the type policy.

Here's it is for reference (the relevant code is in the first few lines):

/**
 * The original `relayStylePagination` function assumes that `edges` and `pageInfo` are defined but
 * we don't always query these fields from our Relay pagination-compatible fields.
 */
export function relayStylePagination<TNode = Reference>(
  keyArgs?: FieldPolicy<any>['keyArgs'],
): RelayFieldPolicy<TNode> {
  const { keyArgs: policyKeyArgs, read, merge } = originalRelayStylePagination<TNode>(keyArgs)

  return {
    keyArgs: policyKeyArgs,
    read(existing, options) {
      if (existing == null || existing.edges == null || existing.pageInfo == null) return existing

      return read?.(existing, options)
    },
    // @ts-expect-error: The incoming type doesn't match the return type.
    merge(existing, incoming, options) {
      if (incoming == null) return existing

      if (existing == null) return incoming

      if (
        existing.edges == null ||
        existing.pageInfo == null ||
        incoming.edges == null ||
        incoming.pageInfo == null ||
        typeof merge !== 'function'
      ) {
        return options.mergeObjects(existing, incoming)
      }

      return merge(existing, incoming, options)
    },
  }
}

[migueloller]

FWIW, we had to make two modifications to make this wrapper ultimately work:

• Delete the read function because it was clobbering pageInfo.endCursor (See relayStylePagination read method clobbers pageInfo.startCursor and pageInfo.endCursor #8267).
• Modify keyArgs function because its value of false is of course needed to exclude pagination arguments but it's also too coarse because there could be other arguments other than pagination arguments (e.g., filters) and it was causing data to be undefined in components using useQuery event though loading was false and there were no errors.

    keyArgs(args) {
      if (args == null) return []

      return Object.keys(args).filter(key => !['first', 'after', 'last', 'before'].includes(key))
    },

[benjamn]

Fix incoming, based on @bubba's PR #7949 (and incorporating @migueloller's code above): #8733

@SirensOfTitan Please take a look, and sorry for the wait!

@migueloller I'm optimistic that the {existing,incoming}.{edges,pageInfo} == null checking is not really necessary, since those properties are checked for truthiness before they're used, but please feel free to comment on #8733 if you think additional changes are needed.