AC3: useQuery does not return old data anymore when variable change

[macrozone]

Intended outcome:

useQuery did return old data when variables changed. This was confusing, i admit, but was very useful.

see #6039

Actual outcome:

useQuery has no longer this behavior. this was changed last minute and is not mentioned as a breaking change #6566

because it is not mentioned, i consider it a bug. There should be a flag to restore this behaviour

How to reproduce the issue:

see #6039 but reverse ;-)

Versions

  System:
    OS: macOS 10.15.5
  Binaries:
    Node: 12.13.1 - ~/.nvm/versions/node/v12.13.1/bin/node
    Yarn: 1.21.1 - ~/git/panter/veloplus/veloplus-shop/veloplus-shop-fe/node_modules/.bin/yarn
    npm: 6.12.1 - ~/.nvm/versions/node/v12.13.1/bin/npm
  Browsers:
    Chrome: 83.0.4103.116
    Firefox: 76.0.1
    Safari: 13.1.1
  npmPackages:
    @apollo/client: 3.0.0 => 3.0.0 
    @apollo/link-persisted-queries: ^1.0.0-beta.0 => 1.0.0-beta.0 
    apollo: ^2.28.3 => 2.28.3 
    apollo-link-schema: ^1.2.5 => 1.2.5 
    apollo-upload-client: ^13.0.0 => 13.0.0 

Edit: it is mentioned, but with a confusing wording.

[benjamn]

I hear you, but this was very much an intentional change, as I hope you can tell from the discussion that motivated it.

That said, the real problem with redelivering previous data was that it was delivered in the same way as current data (via result.data), so there was no way to tell if the data was fresh or stale. What do you think about providing result.previousData (or even a chain of result.previousResult.previousResult...) to allow application code to explicitly choose to reuse old data, when convenient?

[macrozone]

I hear you, but this was very much an intentional change, as I hope you can tell from the discussion that motivated it.

That said, the real problem with redelivering previous data was that it was delivered in the same way as current data (via result.data), so there was no way to tell if the data was fresh or stale. What do you think about providing result.previousData (or even a chain of result.previousResult.previousResult...) to allow application code to explicitly choose to reuse old data, when convenient?

I know, I even participated in these discussions and critized this problem of AC2 ;-) But now after upgrading to AC3, i saw that this behaviour is actually a useful default and that we relied on it in so many places.

having previousData would surely be a good solution. Having additionaly a flag to restore the old behaviour would also help developers to migrate, but for me, previousData would be enough (tbh. i was expecting that previousData already exists)

To mimic this, you can do this:

import { usePreviousDistinct } from "react-use";

// 
const {data, error, loading} = useQuery(...)
const previousData = usePreviousDistinct(data, (p, next) => !next);

const whatINeed = (data ?? previousData)?.myStuff

having previousData already in the result of useQuery would be much cleaner.

usePreviousDistinct(data, (p, next) => !next); is cryptic. Unfortunatly const previousData = usePrevious(data) does not work, altough this function exists in react-use. It will not work the same as AC2: when variables change while the previous query is still in-flight, it will update previousData value with undefined

[benjamn]

I'm not generally a fan of flags that let people avoid adapting their code, since the end result of that philosophy is death by a thousand flags, so it's good to hear that you'd be open to result.previousData.

I hadn't seen react-use before, so thanks for letting me know that exists. It sounds like it could be a good tool for implementing workarounds like this, in some situations at least.

[andyrichardson]

So I just found out about this change - not a huge fan 😞

Is the assumption that most projects are going to start storing (and updating) responses in their own local state rather than using the cache as the single source of truth? Sounds like a lot of new useState and useEffects are on their way!

Is it possible to get this documented in big red letters in the hooks migration guide as I couldn't seem to find any mention of this breaking change 📖

P.S. Thanks for all your hard work on Apollo 😘

[hueter]

Also if you don't want a third party library, here is a custom hook that I've been using to track the previous value:

import { useRef, useEffect } from "react";

/**
 * A modification of usePrevious, this hook only keeps track of the last
 *  non-nullish value
 */
export const usePreviousNonNullish = <T>(value: T): T => {
  const ref = useRef<T>(value);
  useEffect(() => {
    if (value !== null && value !== undefined) {
      ref.current = value;
    }
  });
  return ref.current;
};

This accounts for the fact that data can be undefined twice in a row

const { data } = useQuery(ANYTHING);
const prevData = usePreviousNonNullish(data);
const existingData = data ?? prevData;

[jfrolich]

I hear you, but this was very much an intentional change, as I hope you can tell from the discussion that motivated it.

That said, the real problem with redelivering previous data was that it was delivered in the same way as current data (via result.data), so there was no way to tell if the data was fresh or stale. What do you think about providing result.previousData (or even a chain of result.previousResult.previousResult...) to allow application code to explicitly choose to reuse old data, when convenient?

Sound like an unnecessary potential memory leak (or am I wrong?). I was also surprised by this change. But it's trivial to store the previous result in a ref. I don't think there is a reason to provide it in a payload if it can easily be done in userland.

[jfrolich]

@hueter: No need for a useEffect here!

[hueter]

@jfrolich ah 🤔 hmmm interesting, it might not make a noticeable difference in this case, however I was following Dan Abramov's advice from here (near the bottom):

Generally, you should avoid reading or setting refs during rendering because they’re mutable. We want to keep the rendering predictable. However, if we want to get the latest value of a particular prop or state, it can be annoying to update the ref manually. We could automate it by using an effect...

I think in my example the useEffect will update the value after the render has taken place, while if we remove it, it will update it during the rendering phase. Probably doesn't matter much either way though 😅

[ivan-kleshnin]

@jfrolich never do effects bigger than console.log in render directly. You'll get problems with SSR, with React Fiber and others.

[jfrolich]

This might be a side-effect but I don't see why this particular assignment should cause any problems in server rendering or concurrent mode, and I know the react reconciler pretty well. But maybe I am missing something.

[maapteh]

I cant find the rationale behind this change, is there anywhere a thread?

In our app we have filtering for example, and to simply rely on what is in cache and when its loading was great for us developers and our visitors. Now we are supposed to store the previous response on the query? That means a lot of useEffects which have to be added:( And why? Same about the loading state suddenly being true even when having its data aggregated on the server.

Personally i would like to read about these changes in the migration documentation, instead of only the changes involved. Because upgrading was easy, the new behaviour was surprisingly new.

That being said, im very happy the new version is out, congratulations

[jfrolich]

You can use this snippet, there is really not a lot of code involved:

+ let cachedData = React.useRef(undefined);
let { data } = useQuery(..);
+ if (data) cachedData.current = data;

Below just use cachedData.current.

[maapteh]

But i simply used data.foo everywhere. I can do stuff, im a frontender. But it's not always good to do stuff. Especially when the query takes variables where i could decide to show part of old state and spinner or only a spinner. Now i have to hustle with data and i want to understand the rationale of the change.

[Nohac]

I'm also not a fan of this change. I've used the previous behavior to my advantage pretty much everywhere. I always show a "full page" loading indicator on initial load (when data is undefined and loading is true), then, on subsequent loads (variable change etc.) I show a partial loading indicator (e.g. a small spinner in the corner or something).

With this change, all my previous work becomes a twitchy and jumpy nightmare to use due to the "full page" loading always showing up, and changing this everywhere would require a lot of work.

I think I understand the issue with not wanting to show a loading indicator if the data returned is actually from the cache (using fetch policy "cache-and-network"), but this seems like such a niche use case to me..

In my opinion, instead of returning "previousResult", add a variable that indicates the current cache state of the data, that way, I think all use cases mentioned could be solved like this:

const { data, loading, cacheStatus } = useSearchQuery({ variables: { search: ... }});
if (data == undefined && loading == true) {
  // show initial loading indicator (cacheStatus = "none")
}

if (loading == true && cacheStatus == "cached") {
  // don't show loading indicator
}

if (loading == true && cacheStatus == "invalidated") {
  // show loading
}

I'm sorry if this comment comes off as "ignorant" or "arrogant", I didn't spend to much time reading through the previous issues, so I'm not sure if I fully understand the problem described there.

[macrozone]

I'm not generally a fan of flags that let people avoid adapting their code, since the end result of that philosophy is death by a thousand flags, so it's good to hear that you'd be open to result.previousData.

flags are bad, i know. But maybe its currently the best solution to have one global flag that restores the old behavior, so that people get a chance to migrate to apollo v3.

The change was added last minute where we already had a release candidate, so it took many by surprise. So maybe a compromise is necessary.

I have one project that uses apollo v2 and we decided not to update it until apollo v3 has a solution for that. I don't want to "decorate" every useQuery with a usePrevious or similar or replace useQuery with our own function that includes usePrevious. I want to avoid that if possible.

adding previousData would also be ok for me and is surely the better solution concerning api, but be aware that this still requires a lot of manual work and finding the places where this is needed is not that easy. It needs careful testing of the full application with every possible change of variables.

[madjam002]

I too was confused by this when upgrading, although the change does make sense as returning stale data isn't always a good idea.

Instead of previousData, how about a simple boolean option such as returnStaleData (just like returnPartialData)? That way you can opt in to the old behaviour where it makes sense. The two places where I display stale data is on a search filtering page, and on a map, where it is useful to display the old results until the new results have finished loading. A loading spinner can be displayed on top of the old results by conditionally rendering on result.loading, so displaying stale data isn't necessarily a bad design pattern.

[macrozone]

I too was confused by this when upgrading, although the change does make sense as returning stale data isn't always a good idea.

Instead of previousData, how about a simple boolean option such as returnStaleData (just like returnPartialData)? That way you can opt in to the old behaviour where it makes sense. The two places where I display stale data is on a search filtering page, and on a map, where it is useful to display the old results until the new results have finished loading. A loading spinner can be displayed on top of the old results by conditionally rendering on result.loading, so displaying stale data isn't necessarily a bad design pattern.

i also came to that conclusion that i actually liked the old behaviour and was not aware that i relied so heavily on it!

having a flag per userQuery would also be my prefered solution as it is straight forward to migrate (as opposed to add (data ?? previousData) everywhere)

[finnhvman]

Hello, we are having the same issue in multiple projects. I'm in favor of having a flag for useQuery where we can specify if we want to keep stale data. Thank you!

[tyler-dot-earth]

Just wanted to note that:

1. using a ref a la @jfrolich's suggestion worked fine for me.

2. It would be great if this were better documented as a breaking change between v2 and v3 (if it was documented, i completely missed it). I'll drop a comment in the appropriate issue.

[prcdpr]

Drop in replacement for useQuery

import { useRef } from "react";
import { useQuery, DocumentNode, QueryHookOptions } from "@apollo/client";

export default function useQueryStale<TData, TVariables>(
  query: DocumentNode,
  options?: QueryHookOptions<TData, TVariables>
) {
  let cachedData = useRef<TData | undefined>(undefined);

  const queryResult = useQuery<TData, TVariables>(query, options);

  if (
    queryResult.loading !== true &&
    queryResult.data !== undefined &&
    // Check for empty object due to https://github.com/apollographql/apollo-client/issues/6876
    Object.keys(queryResult.data).length > 0
  ) {
    cachedData.current = queryResult.data;
  }

  return { ...queryResult, data: cachedData.current };
}

[lukewlms]

Here's our current solution, seems to be working well. (Replaces useQuery.)

The point is just to ensure that once data is defined, it's never reset to undefined.

export function useQueryWithTypes<TData, TVariables>(
  query: TypedDocumentNode<TData, TVariables>,
  options?: QueryHookOptions<TData, TVariables>,
) {
  const result = useQuery(query, options);

  const [data, setData] = useState<TData | undefined>(undefined);
  useEffect(() => {
    if (result.data !== undefined) {
      setData(result.data);
    }
  }, [result.data]);

  return { ...result, data };
}

[benjamn]

I'm happy to share that we finally have an implementation of result.previousData (#7082), and you can test it now using @apollo/client@3.3.0-beta.6.

We recommend the idiom result.data ?? result.previousData to obtain the most recent useful data (if you're comfortable with it possibly being stale). While this may seem like more code than just result.data, the ?? expression represents a choice that should be made consciously (specifically, that you're okay with stale data), and explicitly enabled with a minimum of extra syntax. We hope result.previousData gives you the power to make that choice, while ensuring result.data is never ambiguously stale.

[mdugue]

@benjamn works like a charm 🎉, thanks a lot!

Some details I stumbled upon that might be interesting for others:

• I had to restart my create-react-app based dev environment, otherwise previousData was always undefined (not sure why)
• I ended up doing

const {previousData, data = previousData,} = useQuery()
data?.doSthHere

for the cases where stale data is fine

[oceandrama]

@benjamn, unfortunately, it doesn't work for me, I always have previousData as undefined in useLazyQuery

[michael-land]

@benjamn, unfortunately, it doesn't work for me, I always have previousData as undefined in useLazyQuery

same here for 3.3.0.rc

[benjamn]

@oceandrama @xiaoyu-tamu Let's continue the discussion about useLazyQuery over at #7396. Thanks for bringing that problem to our attention.

[HeyParkerJ]

While I don't yet have on opinion on the quality of the change, I find it surprising that this isn't mentioned in the Apollo v3 migration docs. @benjamn, is there a roadmap to updating the migration docs with all of the intentionally backwards incompatible changes, or a rationale for not including them otherwise?

[ericArbour]

Would it be possible to have previousData added to useMutation as well? If so, I'd be happy to open a new issue for it.

[jzaynal]

@mdugue

• I ended up doing

const {previousData, data = previousData,} = useQuery()
data?.doSthHere

for the cases where stale data is fine

Cheers mate, that is a nice trick. And more importantly that was the previous behaviour with v2.x anyway, right? so this works very nicely for anyone migrating to v3 and not wanting to mess too much to adapt to latest practices. So thanks!

[nathanredblur]

In order to have our previous behavior and prevent unexpected issues.
we implemented this hook. I hope this helps someone or open a discussion to find a better way.

import { useQuery } from '@apollo/client';

export const useQueryFix = (query, options) => {
  const { data, ...queryResult } = useQuery(query, options);

  let newData = data;
  if (queryResult.error) {
    if (data === null) newData = undefined;
  } else if (!data) newData = queryResult.previousData;

  return {
    data: newData,
    ...queryResult,
  };
};

[ryparker]

Here's a Typescript version of nathanredblur@'s hook.

import {
  DocumentNode, OperationVariables, QueryHookOptions, QueryResult, TypedDocumentNode, useQuery
} from '@apollo/client';

/**
 * This is customization of the Apollo useQuery hook.
 *
 * This hook is just like useQuery except when loading, instead of
 * returning undefined, it returns the previous query's data.
 *
 * @see https://github.com/apollographql/apollo-client/issues/6603
 */
const usePersistentQuery = <TData = any, TVariables = OperationVariables>(
  query: DocumentNode | TypedDocumentNode<TData, TVariables>,
  options?: QueryHookOptions<TData, TVariables>
): QueryResult<TData, TVariables> => {
  const { data, ...queryResult } = useQuery(query, options);
  let newData = data;

  if (queryResult.error && data === null) {
	newData = undefined;
  }

  if (!queryResult.error && !data) {
    newData = queryResult.previousData;
  }

  return {
    data: newData,
    ...queryResult,
  };
};

export default usePersistentQuery;