fetchMore weird side effects when altering previousResult object

[bobbybobby]

I spent a lot of time dealing with erratic behaviour from fetchMore and I finally realized it was because I was altering the previousResult object in the updateQuery function. I'm posting this issue because I don't know if it's an intented behaviour (do not touch previousResult) or not. And if it is, I found no documentation or discussion about this, so I hope this issue will help future people stumbling on this.

How to reproduce the issue:

This is the gist of what I was doing :

import _ from 'lodash';

const ActorsQuery = gql`
  query Actors_Query($first: Int, $after: String) {
    actors(first: $first, after: $after) @connection(key: "actors") {
      edges{
        node{
          __typename
          id
          displayName
          avatar
        }
      }
      pageInfo{
        hasNextPage
        endCursor
      }
    }
  }
`;

<Query
  query={ActorsQuery}
  variables={{
    first: 5
  }}
  notifyOnNetworkStatusChange={true}
>
  {({ loading, error, data, refetch, fetchMore}) => {
    
  console.log(`render called with loading = ${loading}`);

    <Actors {...this.props} {...data} loading={loading}
      refetch={refetch}
      onLoadMore={() =>
        fetchMore({
          variables: {
            after: _.get(data, `actors.pageInfo.endCursor`),
            first: 5
          },
          updateQuery: (previousResult, { fetchMoreResult }) => {
            const previousEdges = _.get(previousResult, 'actors.edges');
            const newEdges = _.get(fetchMoreResult, 'actors.edges');
            const pageInfo = _.get(fetchMoreResult, 'actors.pageInfo');
    
            let newResult = Object.assign({}, _.set(previousResult, 'actors', {
              __typename: _.get(previousResult, 'actors.__typename'),
              edges: [...previousEdges, ...newEdges],
              pageInfo
            }));
    
            console.log(`updateQuery with new result ${JSON.stringify(newResult)}`);
            return newResult;
          }
        })
      }
    />
  }}
</Query>  

Actual outcome:

As you can see, in updateQuery I was first mutating previousResult (by means of _.set()), then assigning it to a new instance to return. As a result, each time I triggered onLoadMore(), updateQuery was successfully called (and returned the right data), however the Query render() function was sometimes called, sometimes not (I couldn't reproduce a predictable pattern, things like adding a delay to the graphql server response made it work, or navigating in my app with react router and coming back made the pagination work when it didn't before).

Intended outcome:

Either :

• it works, and when triggerring onLoadMore() pagination works as expected with the render() function being called with loading: true then with loading: false and the new result.
• it is easy to find somewhere in the documentation or in online discussion that you should not touch the previousResult object.

I fixed this by not mutating the previousResult object :

  let newResult = Object.assign({}, previousResult);
  _.set(newResult, 'actors', {
    __typename: _.get(previousResult, 'actors.__typename'),
    edges: [...previousEdges, ...newEdges],
    pageInfo
  });

Versions

  System:
    OS: Linux 4.15 Ubuntu 18.04.2 LTS (Bionic Beaver)
  Binaries:
    Node: 8.10.0 - /usr/bin/node
    Yarn: 1.13.0 - /usr/local/bin/yarn
    npm: 6.7.0 - /usr/local/bin/npm
  Browsers:
    Firefox: 66.0.3
  npmPackages:
    react-apollo: 2.5.1 => 2.5.1 

[benjamn]

In the next version of apollo-cache-inmemory (1.6.0, currently 1.6.0-beta.6), there will be an option to Object.freeze the result objects read from the cache (in development), which should help prevent this kind of weirdness: #4514

To complement new InMemoryCache({ freezeResults: true }), the ApolloClient constructor will also soon accept an assumeImmutableResults option (in [email protected]), which takes advantage of the immutability to avoid recording defensive snapshots of data: #4543

Putting these two new options together makes for a pretty good solution to the problems you described:

const client = new ApolloClient({
  link: ...,
  cache: new InMemoryCache({
    freezeResults: true, // new
  }),
  assumeImmutableResults: true, // new
});

Once you embrace immutable cache results, you can tell if two result objects are the same using === rather than actually comparing their contents.

Longer term, I would love for immutability to be the default, but of course that would be a breaking change for many applications, so for now it needs to remain optional.

[awmottaz]

I, too, spent a long time trying to figure out this problem before finding this solution. Can we please get a documentation update to point this out?

[hwillson]

Please try a recent version of @apollo/client and let us know if this issue is still happening. Thanks!