Apollo-client: 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;
  }
});

1. 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
});

1. 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 https://github.com/apollographql/react-apollo/pull/2889#issuecomment-482580577 .

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 https://github.com/apollographql/react-apollo/pull/1639#issuecomment-465766827, 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

[hueter]

Hi all, I just wanted to add to this issue because although this change is reasonable, it reverses a major pattern from the past 2 years of Apollo client and there are probably a handful of apps like mine that didn't realize this behavior was unintentional until we upgraded and found this issue.

In my app, we had deliberately used this behavior so the user could still view a table of data instead of a loading spinner. For example, consider:

const { data, loading } = useQuery(EXAMPLE_QUERY, { variables: variablesFromQueryParams });

if (data) {
  return <TableComponent loading={loading} contacts={data.contacts} />;
}

return <LoadingSpinner />;

The loading state of the TableComponent above was just to change the opacity, so all of the previous data was still visible while loading. The <LoadingSpinner /> was supposed to be a fallback for basically the first page load only.

With #6566, now we see a loading spinner any time the query updates, no more seeing the opacity change on the existing data (as can be expected)

Luckily in our repo we have a handy custom hook called usePreviousNonNullish (inspired by this and this blog post) that keeps a ref to the prior version of a variable so I was able to re-implement this feature like so:

Custom Hook

export const usePreviousNonNullish = <T>(value: T): T => {
  const ref = useRef<T>(value);
  useEffect(() => {
    if (value !== null && value !== undefined) {
      ref.current = value;
    }
  });
  return ref.current;
};

Usage

const { data, loading } = useQuery(EXAMPLE_QUERY, { variables: variablesFromQueryParams });
const prevData = usePreviousNonNullish(data);

const contactData = data ?? prevData; // fall-back to the previous data while 'data' is undefined

if (contactData) {
  return <TableComponent loading={loading} contacts={contactData.contacts} />;
}

return <LoadingSpinner />;

So I mainly wanted to leave this example here in case anyone upgrades and their stuff isn't working anymore.

But I also wanted to follow up with @benjamn because originally @davismariotti suggested adding a possible query option to preserve this behavior. Should I go ahead and track my own previous data or would this be something apollo could provide? Or maybe I'd be able to leverage the cache for this?

Thank you 🙏, apologies for commenting on the closed issue

[macrozone]

Hi all, I just wanted to add to this issue because although this change is reasonable, it reverses a major pattern from the past 2 years of Apollo client and there are probably a handful of apps like mine that didn't realize this behavior was unintentional until we upgraded and found this issue.

In my app, we had deliberately used this behavior so the user could still view a table of data instead of a loading spinner. For example, consider:

const { data, loading } = useQuery(EXAMPLE_QUERY, { variables: variablesFromQueryParams });

if (data) {
  return <TableComponent loading={loading} contacts={data.contacts} />;
}

return <LoadingSpinner />;

The loading state of the TableComponent above was just to change the opacity, so all of the previous data was still visible while loading. The <LoadingSpinner /> was supposed to be a fallback for basically the first page load only.

With #6566, now we see a loading spinner any time the query updates, no more seeing the opacity change on the existing data (as can be expected)

Luckily in our repo we have a handy custom hook called usePrevious (inspired by this and this blog post) that keeps a ref to the prior version of a variable so I was able to re-implement this feature like so:

const { data, loading } = useQuery(EXAMPLE_QUERY, { variables: variablesFromQueryParams });
const prevData = usePrevious(data);

const contactData = data ?? prevData; // fall-back to the previous data while 'data' is undefined

if (contactData) {
  return <TableComponent loading={loading} contacts={contactData.contacts} />;
}

return <LoadingSpinner />;

So I mainly wanted to leave this example here in case anyone upgrades and their stuff isn't working anymore.

But I also wanted to follow up with @benjamn because originally @davismariotti suggested adding a possible query option to preserve this behavior. Should I go ahead and track my own previous data or would this be something apollo could provide? Or maybe I'd be able to leverage the cache for this?

Thank you 🙏, apologies for commenting on the closed issue

thank you.

usePrevious is also available in react-use, but it does not fully restore the old behaviour: If variable changes, while previous query is still in flight, it will get updated with empty data.

/

[hueter]

hi @macrozone that is a good point, I updated my comment and also replied to the other thread with a working solution that modifies the usePrevious hook slightly to only keep track of the previous non-nullish value. This accounts for the fact that data can be undefined twice in a row.

[delyanr]

@benjamn Sorry to bring this back, but it seems like the problem is (rather ironically) not fixed for fetchPolicy: 'no-cache'. For a reproduction, you can simply go into OP and add the fetchPolicy: 'no-cache'. I tried it with both version 3.1.3 and 3.2.0-beta4. Thanks!