When variables change, data is the previous result in Query Component

[davismariotti]

Continuing discussion from the bug apollographql/react-apollo#2202

Intended outcome:
I want the data returned from useQuery to be undefined when the variables change. Reasoning is that if the variables for the query change, loading is set to true, but data remains set to the data from the previous query with old variables.

Actual outcome:
data is set to the previous variables data while the next query (with new variables) is in flight.

How to reproduce the issue:
https://codesandbox.io/s/changing-variables-demo-obykj

Discussion
While this may be intended and desired for most cases, I think it would be a good change to add the option for developers to choose if they want this behavior. One example (that caused me to find this issue) was a search bar that searches as you type, but doesn't search for anything (and hides results) if you've only typed less than 3 characters. My issue was that the user could search for something, delete everything, then re-search with different text. When the third character was pressed, skip would be set to false, and the search would happen, but there would be a brief moment of showing the data from the old query variables.

I looked at apollographql/react-apollo#2889 by @jcready which had a potential fix for this issue, but looks abandoned. But following the discussion, a few possibilities were discussed as potential fixes to this issue:

1. Adding a shouldInvalidatePreviousData option to useQuery.
   This would allow a developer to write code such as:

const { data, loading, error } = useQuery(FIND_DOGS, {
  variables: {
    search: searchText
  },
  shouldInvalidatePreviousData(nextVariables, previousVariables) {
    return nextVariables !== previousVariables;
  }
});

2. Adding a clearPreviousDataOnLoad option to useQuery.
   This would allow a developer to write code such as:

function usePrevious(value) {
  const ref = useRef();
  useEffect(() => {
    ref.current = value;
  }, [value]);
  return ref.current;
}

...

const previousSearchText = usePrevious(searchText);
const { data, loading, error } = useQuery(FIND_DOGS, {
  variables: {
    search: searchText
  },
  clearPreviousDataOnLoad: searchText !== previousSearchText
});

3. A final option that I see as more of a workaround that requires no changes:
   https://codesandbox.io/s/cool-brown-ttwxw

function usePrevious(value) {
  const ref = useRef();
  useEffect(() => {
    ref.current = value;
  }, [value]);
  return ref.current;
}

...

const previousSearchText = usePrevious(searchText);
const { data, loading, error } = useQuery(FIND_DOGS, {
  variables: {
    search: searchText
  }
});

const showDataWhileLoading = searchText === previousSearchText
const updatedData = !loading || showDataWhileLoading ? data : undefined;

I feel the third option is weird to write, and while the first option is probably the easiest from a developer's perspective, the second option would be the easiest to implement

Versions

System:
  OS: macOS 10.15.3
  Binaries:
    Node: 10.18.1 - /usr/local/bin/node
    Yarn: 1.21.1 - ~/.npm-global/bin/yarn
    npm: 6.13.1 - ~/.npm-global/bin/npm
  Browsers:
    Chrome: 80.0.3987.132
    Firefox: 69.0.2
    Safari: 13.0.5
  npmPackages:
    @apollo/client: ^3.0.0-beta.39 => 3.0.0-beta.39
    @apollo/react-components: 3.2.0-beta.0 => 3.2.0-beta.0
    @apollo/react-hoc: 3.2.0-beta.0 => 3.2.0-beta.0
    apollo-cache-persist: ^0.1.1 => 0.1.1
    apollo-client: ^2.6.4 => 2.6.8
    apollo-link-error: ^1.1.12 => 1.1.12
  npmGlobalPackages:
    apollo: 2.21.3```

[davismariotti]

Because @hwillson had proposed the second option in apollographql/react-apollo#2889, I created a pull here: #6040

[fh-jashmore]

I have this same scenario

[jtomaszewski]

I think sth like shouldInvalidatePreviousData should be the preferred solution, for the same reasons as said in apollographql/react-apollo#2889 (comment) .

For interested, I made a small utility function in which you can wrap your useQuery(...) call, so that the data is cleaned if it comes from the previous query. Of course treat it as a last resort hacky workaround. (It requires you to pass variables twice, once to useQuery and once to that util function; I'm not sure also if it will work with options like skip.)

import { NetworkStatus } from "apollo-client";
import { isEqual } from "apollo-utilities/lib/util/isEqual";
import { useRef } from "react";
import { QueryResult } from "react-apollo";

/**
 * Use this if you want your `useQuery()` hook to not return previous query's result `data`
 * after you change the `variables` to fetch a new query.
 *
 * This is temporary workaround until `@apollo/react-hooks` provides a solution for
 * https://github.com/apollographql/apollo-client/issues/6039 .
 *
 * @example
 * ```ts
 * const { loading, data, refetch } = useClearPreviousDataOnVariablesChange(
 *   useGetUpcomingBillsQuery({
 *     variables: { reference }
 *   }),
 *   { reference }
 * );
 * ```
 */
export function useClearPreviousDataOnVariablesChange<TData, TVariables>(
  result: QueryResult<TData, TVariables>,
  variables: TVariables
): QueryResult<TData, TVariables> {
  const prevResultRef = useRef(result);
  const prevVariablesRef = useRef(variables);
  const resultHasBeenLoadingRef = useRef(false);

  // Variables have changed, but data is the same as it was for the previous variables
  if (
    !isEqual(prevVariablesRef.current, variables) &&
    prevResultRef.current.data &&
    prevResultRef.current.data === result.data
  ) {
    // If the result is loading,
    // return empty data and mark that result has been loading
    if (result.loading) {
      resultHasBeenLoadingRef.current = true;
      return { ...result, data: undefined };
    }

    // If the result has loaded successfully,
    // return it
    if (
      !result.loading &&
      resultHasBeenLoadingRef.current &&
      result.networkStatus === NetworkStatus.ready
    ) {
      prevResultRef.current = result;
      prevVariablesRef.current = variables;
      resultHasBeenLoadingRef.current = false;
      return result;
    }

    // If the result has failed to load,
    // return empty data
    if (
      !result.loading &&
      resultHasBeenLoadingRef.current &&
      result.networkStatus === NetworkStatus.error
    ) {
      return { ...result, data: undefined };
    }
  }

  prevResultRef.current = result;
  prevVariablesRef.current = variables;
  resultHasBeenLoadingRef.current = false;
  return result;
}

[benjamn]

@davismariotti Just to make sure I understand your expectations, would it be fair to say you never want data to be the (unrelated) previous data, but data could be something other than undefined when variables change? For example, if the new variables produce a cache hit, you'd be ok with getting a loading: true result with data from the cache?

Looking at your reproduction, I noticed you're using an object with __typename: "RedditAPI" as the Query.reddit field, but the cache doesn't know how to merge those RedditAPI objects together, so each query ends up overwriting the fields returned by the previous query. This is something we now warn about (#6372), if you update to a more recent beta/RC.

One solution is to bless the RedditAPI type as a singleton object, so your books, movies, and news fields will accumulate over time, rather than getting replaced every time Query.reddit is written. This will allow the cache to satisfy your queries without going to the network:

const client = new ApolloClient({
  link: new HttpLink({
    uri: "https://www.graphqlhub.com/graphql"
  }),
  cache: new InMemoryCache({
    typePolicies: {
      RedditAPI: {
        // This means the identity of the RedditAPI object doesn't depend on any
        // of its fields other than its __typename, which allows the cache to merge
        // any/all RedditAPI fields into the same singleton object.
        keyFields: [],
      },
    },
  }),
});

I realize that's not the answer to this particular issue, though it does improve the cache behavior of the reproduction. Just thought I'd mention it to make sure you're aware of your options here.

[benjamn]

In general, I agree with @clayne11's comment apollographql/react-apollo#1639 (comment), and I would love to stop delivering previous data at all, without any special configuration around when it might be safe to deliver.

[benjamn]

Though this may be a breaking change, I believe it's really important to get this fixed before AC3 is released: #6566