apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.37k stars 2.66k forks source link

readField() in type policy merge function returns undefined only on first query #9315

Open miracle2k opened 2 years ago

miracle2k commented 2 years ago

I was using @apollo/client@3.3.19 with a custom type policy to do pagination merging based on what is described here:

https://www.apollographql.com/docs/react/caching/cache-field-behavior/#merging-non-normalized-objects

My merge function looks as follows. It is intended to merge a connection-type entity with a nodes subkey, so something like:

query { 
  platform {
      events {
           nodes {
               id
           }
      }
  }
}

Here it is:

merge(existing: any, incoming: any, { args, readField }: any) {
      const { nodes: incomingNodes, ...rest } = incoming || {};

      let mergedNodes: Map<string, any>;
      if (existing?.nodes) {
        mergedNodes = new Map(existing.nodes);
      } else {
        mergedNodes = new Map();
      }

      incomingNodes?.forEach((item: any) => {
        mergedNodes.set(readField("id", item), item);
      });

      return {...rest, nodes: mergedNodes};
    },

This worked fine. After upgrading to @3.5.7 I see the following behaviour:

  1. The first time useQuery() gets a result, readField("id", item) returns undefined. Because of this, only a single node is actually stored in the cache.
  2. On subsequent requests (say to load the second page), readField works correctly.

I don't have a repo, but wonder if it is related to https://github.com/apollographql/apollo-client/pull/8508/files

Note: nodes is an interface type called Event, the nodes returned from the server are a concrete type implementing the interface (SaleEvent). I am defining possibleTypes in the cache (it implements both interfaces):

{
   Event: ['SaleEvent'],
   MarketplaceEvent: ['SaleEvent'],
}
BogdanAdrian commented 1 year ago

I'm experiencing the same issue on @apollo/client: 3.7.1 (latest release right now). Every time I refresh the page, the existing argument from the merge function is coming undefined breaking my infinite scroll 😢

ryancrunchi commented 1 year ago

Same here, with any fetchPolicy involving cache : 'cache-first', 'cache-and-network', 'cache-only'...

lprhodes commented 1 year ago

Just adding that I'm experiencing the same issue, and is the same when setting 'network-only' as the fetchPolicy.

It's not all queries and so I'm digging into specifically what in the graphql output that's causing it to occur. I suspect it's when fetching a connection that contains a field with another connection.

lprhodes commented 1 year ago

It may be different for others but in my case I have the following graphQL structure:

User {
    id
    posts {
        edges [
            {
                node {
                    id
                    message
                    creationTimestamp
                    user {
                        id
                    }
                }

            }}
        ]
    }
}

If the top level User.id is the same as the User.post.edge.node.user.id then readField returns undefined during the first query. The second fetch of the query works as expected.

lprhodes commented 1 year ago

With this issue having been raised almost a year ago, has anyone found a work-around?

lprhodes commented 1 year ago

I've reverted to version 3.3.21 which resolves the issue. 3.4.0 onwards re-introduces it.

m4riok commented 1 year ago

I can confirm that what @lprhodes suggested is correct. This issue only occurs when an object id reappears within the nested objects. It also has another side effect, the read function gets executed twice and the ObservableQuery also fires twice.

lprhodes commented 1 year ago

Unfortunately version 3.3.21 that I downgraded to has an issue where it repeatedly performs queries created by the useQuery hook so I'll need to really dig into this issue.

lprhodes commented 1 year ago

In case anyone's also working on the issue...I'm getting closer to locating the specific code causing the problem. It's related to how the writeToStore function is using the applyMerges function when iterating through the context.incomingById variable.

lprhodes commented 1 year ago

Not a fix but I know specifically what's happening now.

In the case of the following graphql, where the root user.id and nested user.id (i.e. user.id.posts.edges.node.user.id) are the same, the merging of the data is being performed out of order. An attempt is made to mergeuser.posts` before the nodes are saved to the store .

User {
    id
    posts {
        edges [
            {
                node {
                    id
                    message
                    creationTimestamp
                    user {
                        id
                    }
                }

            }}
        ]
    }
}

I'm unsure yet as to whether this is intentional to prevent an infinite loop, a missed requirement or an actual bug.

I'll keep digging and see whether I can determine a fix for it but it'd be much appreciated if @benjamn could chime in please as the author of this piece of code.

lprhodes commented 1 year ago

I've now found the specific code that's causing the issue: https://github.com/apollographql/apollo-client/blob/9134aaf3b6fc398b2d82439b5b63848b533ae4c9/src/cache/inmemory/writeToStore.ts#L420-L434

This code checks whether there's a previous entity and merges the two mergeTree's if so.

I believe this PR is the responsible: https://github.com/apollographql/apollo-client/pull/8372

As a work-around, if you set previous to always equal false then it prevents the nested User object from merging with the root User object (and subsequently causing the User mergeTree to be called out of order).

I assume that by setting previous to always equal false, there will be an impact on performance but I'm yet to see any bugs.

tapnair commented 1 year ago

Having this issue too. If this helps anyone I think I have a decent workaround.
mergedResults[item.__ref] = item; Not ideal, but so far seems reliable.