Types `cache.modify` and `writeFragment` lead to TypeScript errors for documented Apollo examples when specifying generic parameter

[sjdemartini]

Issue Description

As of https://github.com/apollographql/apollo-client/pull/10895 and https://github.com/apollographql/apollo-client/pull/11206, the types for the field modifiers in cache.modify are explicit and more strict, where each field modifier is expected to return a list of references or objects when you specify the generic with cache.modify<MyObjectType>.

However, Apollo's documented examples lead to code that doesn't satisfy this more strict typing, such as https://www.apollographql.com/docs/react/caching/cache-interaction/#example-updating-the-cache-after-a-mutation:

const [addComment] = useMutation(ADD_COMMENT, {
  update(cache, { data: { addComment } }) {
    cache.modify({
      id: cache.identify(myPost),
      fields: {
        comments(existingCommentRefs = [], { readField }) {
          const newCommentRef = cache.writeFragment({
            data: addComment,
            fragment: gql`
              fragment NewComment on Comment {
                id
                text
              }
            `
          });
          return [...existingCommentRefs, newCommentRef];
        }
      }
    });
  }
});

Specifically, in the above, newCommentRef will be Reference | undefined based on writeFragment's longstanding type https://github.com/apollographql/apollo-client/blob/d502a69654d8ffa31e09467da028304a934a9874/src/cache/core/cache.ts#L312-L318.

As such, if you update the above to use cache.modify<CommentType>, this leads to a TS error since undefined is not an allowed return value from a field Modifier function:

Type '(existingCommentRefs: readonly (Reference | AsStoreObject<{...}>)[]) => (Reference | ... 1 more ... | undefined)[]' is not assignable to type 'Modifier<readonly (Reference | AsStoreObject<{...}>)[]>'.
...
        Type 'undefined' is not assignable to type 'Reference | AsStoreObject<{...}>'.ts(2322)

Two questions based on this:

1. Is there a recommended way to handle this?
   • Should we simply use const newCommentRef = cache.writeFragment(...)! (i.e. add !) to mark that the writeFragment return value won't be undefined? It's not clear to me based on the docs when writeFragment would return undefined, but I guess if it's possible to be undefined with normal usage, we'd want to be more defensive and not simply override the type with the ! operator.
2. Can/should the documentation be updated accordingly, and/or perhaps the TS types be adjusted?

Link to Reproduction

Not a runtime bug, just a simple TS error

Reproduction Steps

1. Use TypeScript
2. Use cache.modify<YourObjectType> and a field modifier which returns a value from cache.writeFragment
3. Observe TS error since Reference | undefined is not an allowed field modifier return value, despite documentation suggesting usage of this pattern

@apollo/client version

3.10.4

[alessbell]

Hi @sjdemartini 👋 Thanks for opening this issue - it definitely sounds like the documentation should be updated, but in terms of the TS type of the return value from cache.writeFragment I'll need to dig in a bit more and chat with the team.

We're coming up on a long weekend but I'll get back to you as soon as I can, thanks!

[DmitryMasley]

The problem is not only that writeFragment can possibly return undefined, but the first argument of the modifier can be a Reference. If the return type of the query is the object with say { items: (Reference | ItemObject)[]; totalCount: number } type, you have to check the object itself is not a Reference, though I don't think it is possible for the query itself to be a reference. So the code became more complex in reality in order to satisfy types and not rely on assumptions about cache configuration.

cache.modify({
    fields: {
        itemsList: (
            existingListRef: { totalCount: number; items: Reference[] } | Reference,
            { isReference, readField }
        ) => {
            const newItemRef = cache.writeFragment({
                data: {
                    ...data,
                    __typename: "ItemType"
                },
                fragment: itemFragmentDoc
            });
    
            if (newItemRef) {
                const { items, totalCount } = isReference(existingListRef)
                    ? {
                        items: readField("items", existingListRef),
                        totalCount: readField("totalCount", existingListRef)
                    } as { totalCount: number; items: Reference[] }
                    : existingListRef;

                return {
                    items: [newItemRef, ...items],
                    totalCount: totalCount + 1
                };
            }
            return existingListRef;
        }
    }
})

Though, it may become even worse if you assume that items may not be normalized and be actual array of objects instead of Reference[].

[johanrd]

Yes, I've also stumbled upon this. For later reference (after bisecting) the bureaucratic need to safeguard against existingListRef being a Reference starts popping when upgrading from apollo client v3.7.0 to v3.8.0

@DmitryMasley Many of my cases were handled by using the new cache.modify<T> generic introduced in 3.8, but still need to guard against Reference. See https://github.com/apollographql/apollo-client/issues/12453#issuecomment-2734696969

[DmitryMasley]

Yes, I've also stumbled upon this. For later reference (after bisecting) the bureaucratic need to safeguard against existingListRef being a Reference starts popping when upgrading from apollo client v3.7.0 to v3.8.0

@DmitryMasley Many of my cases were handled by using the new cache.modify generic introduced in 3.8, but still need to guard against Reference. See #12453 (comment)

@johanrd From my perspective passing the generic is almost the same as type casting, only with less writing.

But Reference is still a problem. And this is not a typescript problem. In reality, you can't be sure if any object is a reference or a real object if you don't know what the cache configuration is. These conditions in cache.modify seem redundant and tedious. It would be nice if the cache could handle it internally. Or at least the cache configuration could be inferred with codegen to properly validate the cache.modify

[johanrd]

@DmitryMasley Yes, I very much agree that needing to care for the possible Reference return type is a big pain – it really feels like I'm working against the grain.

Using the generic feels a bit better than casting to me, it especially helped me simplify a bit when removing edges from a connection.

Crossing my finger that getting rid of the Reference return type turns out to be a solvable bug / improvement, as this is now very weird for such a common use case.