Updating cache triggers queries when fetchPolicy: cache-and-network is used

[hinok]

This issue doesn't exist on 3.0.2
This issue exists on >3.0.2

Intended outcome:

Update cache without firing network request while using fetchPolicy: cache-and-network.

Actual outcome:

In the past there was a possibility to update cache without triggering network request by using cache.writeData etc.
Now by using cache.modify, query is re-run and network request is fired.

How to reproduce the issue:

Demo: https://codesandbox.io/s/sleepy-clarke-19944?file=/src/AddUser.js

Open https://e3odl.sse.codesandbox.io/ to start the apollo server used in the reproduction.

Important code

Users.js

import React from "react";
import { gql, useQuery } from "@apollo/client";

const ALL_USERS = gql`
  query AllUsers {
    users {
      id
      name
      active
    }
  }
`;

export default function Users() {
  const { loading, error, data } = useQuery(ALL_USERS, {
    fetchPolicy: "cache-and-network"
  });

  if (loading) {
    return <p>Loading…</p>;
  }

  if (error) {
    return <p>Error</p>;
  }

  return (
    <ul>
      {data.users.map((user) => (
        <li key={user.id}>
          {user.active ? user.name : <s>{user.name}</s>}{" "}
          {user.active ? "Active" : "Not active"}
        </li>
      ))}
    </ul>
  );
}

AddUser.js

import React, { useRef } from "react";
import { gql, useMutation } from "@apollo/client";

const ADD_USER = gql`
  mutation AddUser($user: UserInput!) {
    updateUser(user: $user) {
      id
      name
      active
    }
  }
`;

export default function AddUser() {
  const [mutate, { loading, error }] = useMutation(ADD_USER);
  const nameRef = useRef();
  const activeRef = useRef();

  const handleClick = () => {
    const name = nameRef.current.value;
    const active = activeRef.current.checked;

    mutate({
      variables: {
        user: {
          name,
          active
        }
      },
      optimisticResponse: {
        id: Math.random(),
        name,
        active
      },
      update(cache, { data: { updateUser } }) {
        cache.modify({
          broadcast: false,
          optimistic: false,
          fields: {
            users(existingUsers = []) {
              const newUserRef = cache.writeFragment({
                data: updateUser,
                fragment: gql`
                  fragment NewUser on User {
                    id
                    name
                    active
                  }
                `
              });
              return [newUserRef, ...existingUsers];
            }
          }
        });
      }
    });
  };

  if (error) {
    return <p>Could not add user</p>;
  }

  return (
    <div>
      <div>
        <input ref={nameRef} type="text" name="name" placeholder="Name" />
      </div>
      <div>
        <label>
          <input ref={activeRef} type="checkbox" name="active" />
          Active
        </label>
      </div>
      <div>
        <button disabled={loading} onClick={handleClick}>
          {loading ? "Adding..." : "Add user"}
        </button>
      </div>
    </div>
  );
}

Versions

@apollo/client 3.1.3

[sirugh]

I think I'm seeing a similar behavior when using the merge type policy function. I have a mutation that removes an item from an array and returns the state of that array in the mutation response, but a query observing the array is being re-fired after the cache is updated with the mutation response.

[hinok]

Based on the comment #6760 (comment)

It seems that setting nextFetchPolicy: "cache-first" resolves the issue.

[sirugh]

That fixed it for us! At least half :D We still are seeing some queries being re-fired when others resolve that affect the cache, even after applying the nextFetchPolicy. I think this has to do with a misconfigured merge function.

Edit: I'm not really able to repro this anymore, but I've been working on our merge functions lately and I think I might have "solved" it accidentally. If it happens again I'll try to come back and update this comment, or open a new issue with repro.

[benjamn]

@hinok I see you edited your comment, but I was curious what you meant by "unfortunately optimisticResponse doesn't work in this case." Did you mean there's no way to set a nextFetchPolicy for the optimistic part, or that using nextFetchPolicy makes optimistic updates behave differently, or something else?

To your point about the difference in broadcasting behavior between cache.writeQuery and client.writeQuery in AC2, you can call cache.writeQuery({ ..., broadcast: false }) in AC3 to write data without broadcasting those changes.

[hinok]

@benjamn I edited my comment because I had a small issue on my side in optimisticResponse part of code. I missed __typename if I remember correctly and initially I thought that it's a new behaviour in ac3 or just a bug 😅 . It wasn't bug, it was my fault so ac3 works correctly therefore I edited my comment...

btw it'd be nice to have a warning or info about optimisticResponse that doesn't work because some fields are missing etc.
I'm not sure what broadcast, optimistic options do in cache.modify. I don't see any difference in my demo after setting true/false. Good code examples, demos would be super helpful to understand and explain some of the "new" features of ac3.

If nextFetchPolicy is the official way to solve the problem, I think we can close this issue or we can focus on @sirugh problem.

[benjamn]

Totally agree that we need more examples of all this stuff!

Although nextFetchPolicy is the official solution for now, we are also planning to make some improvements to nextFetchPolicy soon, so that it can optionally be a function that receives the current policy, which I think will make it safer to use in your defaultOptions, so you could reenable the fallback behavior by default across your application, if that's what you prefer.

[benjamn]

@hinok @sirugh With @apollo/[email protected] (which includes #6893), it should be possible to configure nextFetchPolicy globally, using a function instead of just a string:

new ApolloClient({
  link,
  cache,
  defaultOptions: {
    watchQuery: {
      nextFetchPolicy(lastFetchPolicy) {
        if (lastFetchPolicy === "cache-and-network" ||
            lastFetchPolicy === "network-only") {
          return "cache-first";
        }
        return lastFetchPolicy;
      },
    },
  },
})

It's important to use a function here, so you're only modifying cache-and-network and network-only, not any other fetch policies. It would be weird/wrong to change cache-only to cache-first, for example.

Thanks in advance for testing out this new functionality and reporting any problems you encounter!

[afzalsayed96]

@benjamn the problem with setting nextFetchPolicy to cache-first is that the query isn't fired over network anymore even if variables of the query change. This is a big trade-off to adopt the official solution here and I'm sure you are aware of it.

My question - is there a way around this with the new nextFetchPolicy callback syntax or is anything in the works on the roadmap specifically for this?

[sirugh]

the query isn't fired over network anymore even if variables of the query change.

Is this true? Can you repro in codesandbox? The query is effectively "changed" if the variables change, as in, it shouldn't even be the same cache entry because the key changed (unless you have some constant as the key).

[afzalsayed96]

@sirugh it has already been brought up multiple times before

• Apollo Client 3 reloading query after mutation update #6760 (comment)
• [network-only] Unwanted query refetch after mutation breaks user experience #7485 (comment)

I don't think it needs a reproduction

[sirugh]

🤷🏼 Maybe this comment will help you? #7938 (comment)

I'm not particularly invested in this, I just know that a new issue + sandbox repro + clear ask can give the maintainers a good starting point. There are a lot of moving parts in Apollo so a slimmed down repro in sandbox can be very helpful for folks trying to debug this issue. But you do you. Good luck!

[afzalsayed96]

@sirugh Sorry I didn't mean to be passive aggressive.

The issue I mentioned existed in versions after v3.02 and seems to have been resolved in v3.4+ versions. So, it's all good. I guess a repro won't be needed after all and we'll adopt the official solution of setting nextFetchPolicy: cache-first.

Thanks to the Apollo team for their hard work!

[anark]

Is the following global setting still required with the latests @apollo/client? If you do not add this setting what is the result?

@hinok @sirugh With @apollo/[email protected] (which includes #6893), it should be possible to configure nextFetchPolicy globally, using a function instead of just a string:

new ApolloClient({
  link,
  cache,
  defaultOptions: {
    watchQuery: {
      nextFetchPolicy(lastFetchPolicy) {
        if (lastFetchPolicy === "cache-and-network" ||
            lastFetchPolicy === "network-only") {
          return "cache-first";
        }
        return lastFetchPolicy;
      },
    },
  },
})

It's important to use a function here, so you're only modifying cache-and-network and network-only, not any other fetch policies. It would be weird/wrong to change cache-only to cache-first, for example.

Thanks in advance for testing out this new functionality and reporting any problems you encounter!