cache-and-network fetch policy incorrectly reports cache hits as loading

[brainkim]

it('should not appear as loading when there is a `cache-and-network` fetch policy and there is a cache-hit', async () => {
  const query = gql`{ hello }`;

  const cache = new InMemoryCache();
  const link = mockSingleLink(
    {
      request: { query },
      result: { data: { hello: 'from link' } },
      delay: 20,
    },
  );

  const client = new ApolloClient({
    link,
    cache,
  });

  cache.writeQuery({ query, data: { hello: 'from cache' }});

  const { result, waitForNextUpdate } = renderHook(
    () => useQuery(query, { fetchPolicy: 'cache-and-network' }),
    {
      wrapper: ({ children }) => (
        <ApolloProvider client={client}>
          {children}
        </ApolloProvider>
      ),
    },
  );

  // ???????????????????
  // The loading state is true, when I expect it to be false.
  expect(result.current.loading).toBe(true);
  expect(result.current.data).toEqual({ hello: 'from cache' });

  await waitForNextUpdate();
  expect(result.current.loading).toBe(false);
  expect(result.current.data).toEqual({ hello: 'from link' });
});

See 1834f5f

[benjamn]

@brainkim IIRC, you get loading: true because there is still a pending network request for the query?

[puglyfe]

@brainkim IIRC, you get loading: true because there is still a pending network request for the query?

While I believe this is true in practice, based on the number of similar issues I've come across in recent days, it seems like the current behavior doesn't match the spirit of the configuration. There's no clear path for surfacing this state to users that doesn't require refactoring the view logic when using the cache-and-network policy. Compare this to something like refetch, which has a distinct networkStatus

apollo-client/src/core/networkStatus.ts

Line 28 in b214dd1

refetch = 4,

My current workaround for this is wrapping the useQuery hook and slightly overloading the loading logic so that we can continue to implement the hook in our components as usual.

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

/**
 *
 * @param {import('graphql').DocumentNode} query
 * @param {import('@apollo/client').QueryHookOptions} options
 * @returns {import('@apollo/client').QueryResult}
 */
const usePersistedCacheQuery = (query, options) => {
  const result = useQuery(query, {
    ...options,
    fetchPolicy: 'cache-and-network',
  });

  const isActuallyLoading =
    result.loading &&
    !result.client.readQuery({
      query,
      variables: options?.variables,
    });

  return { ...result, loading: isActuallyLoading };
};

But I'd love to have a clear way to indicate there is an active "background fetch" or "refresh" or similar using useQuery.

Other discussions:
apollographql/react-apollo#2601
apollographql/react-apollo#1217
#3660

[cainlevy]

This bug appears to make it impossible to test the loading behavior of cache-and-network components.

The problem is that MockedProvider appears to prime the cache immediately, which fools any workarounds to detect isActuallyLoading by checking for stale data.

edit: I'm no longer confident that I interpreted the test behavior correctly. When I inspected the data returned by my useQuery, it appeared to be unrelated data from a different query.

[conoremclaughlin]

We're also running into issues with cache-and-network incorrectly reporting the loading status (still). We want to refetch only if a network request is not already underway, but with cache hits reporting loading as true, we have no way to detect if a network request is happening when navigating back to a screen, for example.

[jkerrwilson]

I'll +1 this still being an issue, makes using cache-and-network useless if you want to show a loading indicator.

[danieljvdm]

Bumping this issue again, this is a pretty massive UX hole that is difficult to support in its current state

[phryneas]

Hey there - seeing this for the first time since I'm new on the team 👋

I'll try to give my reasoning here - the other team members might disagree:

This is nothing that we could quickly "fix" - adding a new NetworkStatus for this would be a breaking change for all users relying on the current behavior. So it would need to be re-evaluated once we consider the next major version.

That said, I'm unsure if it would be something to reconsider. Having this like it is right now to me looks equally expressive as having a new network status.
This is what we have right now:

	data	loading	networkStatus
mounted for the first time, no data in cache	undefined	true	loading
mounted for the first time, data in cache	value	true	loading
refetching	value	false	refetch

Now, adding another network status won't make this a lot "simpler"

	data	loading	networkStatus
mounted for the first time, no data in cache	undefined	true	loading
mounted for the first time, data in cache	value	false	weHadDataButLoadAnyWays
refetching	value	false	refetch

No matter what we do, depending on how people structure their UI, some will be interested in loading && !data, while others would be interested in the current behaviour of loading. And for those, the loading they had before would become networkStatus == NetworkStatus.loading || networkStatus.weHadDataButLoadAnyWays.
There is no "ergonomic" solution that will satisfy all users.

And if I had to choose between those two options up there, the loading && !data looks more readable to me than checking for multiple network statuses.

[cainlevy]

@phryneas Thank you for the thoughtful analysis and response.

I would suggest that this is a documentation problem. We have learned new patterns for the behavior we want but first we had to discover that "loading" had unexpected behavior. It's such a tempting API but the simplicity doesn't teach developers how to go further or prepare them for other situations that will become painful as their application matures. It's a bit of a honeypot: attractive and easy to get stuck in.

Imagine, then, if documentation (examples, guides, etc) demonstrated networkStatus === "loading" logic instead. It's more typing but also much more revealing. When developers need more nuance, it's there.

[phryneas]

@cainlevy I agree, if that's something where we can improve the DX with documentation, we should definitely do that.
If you have some specific points where you would look first in mind that could benefit from improvements, could you maybe file a pull request?

[ianchanning]

@puglyfe I know comments aren't for just '+1's - but this workaround is absolute magic and emojis don't capture it:

const isActuallyLoading =
    result.loading &&
    !result.client.readQuery({
      query,
      variables: options?.variables,
    });

I've had a thorn in my side for 18 months knowing that something was wrong with the query fetching. Every single page load that should be showing cached data was bringing up loading spinners. But it was a relatively small issue that no-one cared about. Finally seeing the component rendered properly from the cache almost brought a tear to my eye 😂

[ianchanning]

This is probably stupid - but I've created an npm package for this https://github.com/ianchanning/use-cache-network-query - maybe if enough people download it, it will persuade Apollo to change the defaults 😂

[pffigueiredo]

What about doing something as simple as changing the query status ordering comparison to check for data first, wouldn't that do the trick for not showing all the spinners when there is cache? Or are there any obvious CONS that I'm not aware of?

e.g.

Instead of this:

if loading
  return ...

if error
   return ...

if data
    return ...

This 👇

if data
  return ...

if loading
    return ...

if error
   return ...

[ianchanning]

@pffigueiredo the problem I see there is that loading -> error -> data is a very common pattern and a very natural ordering for components, so it would be 'cache-and-network' forcing everyone to change.