apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.34k stars 2.66k forks source link

reproduction around `useQuery` loading with `cache-only` on an empty cache #12021

Open phryneas opened 1 month ago

phryneas commented 1 month ago

This came up in https://community.apollographql.com/t/loading-flips-to-true-at-first-even-with-fetchpolicy-cache-only/7771

I could at least reproduce this in 3.10 and 3.11, so it's not a result of the useQuery rewrite.

The question is also what the correct behaviour would be here.

v2 apparently had loading: false, but data: undefined.

I would argue that a constant loading: true would be preferrable until something else fills in the cache. 🤔

I'll rewrite the test in an older style and do some more bisecting.

changeset-bot[bot] commented 1 month ago

⚠️ No Changeset found

Latest commit: 5dd72ca93ed43c8a49a4522849d183a9e8c4caed

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions[bot] commented 1 month ago

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 39.33 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 47.99 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 45.57 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 34.4 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 32.28 KB (0%)
import { ApolloProvider } from "dist/react/index.js" 1.26 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production) 1.24 KB (0%)
import { useQuery } from "dist/react/index.js" 5.21 KB (0%)
import { useQuery } from "dist/react/index.js" (production) 4.3 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 5.69 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production) 4.77 KB (0%)
import { useMutation } from "dist/react/index.js" 3.62 KB (0%)
import { useMutation } from "dist/react/index.js" (production) 2.84 KB (0%)
import { useSubscription } from "dist/react/index.js" 4.41 KB (0%)
import { useSubscription } from "dist/react/index.js" (production) 3.46 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 5.49 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production) 4.15 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 4.99 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.64 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" 5.07 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production) 3.72 KB (0%)
import { useReadQuery } from "dist/react/index.js" 3.39 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production) 3.33 KB (0%)
import { useFragment } from "dist/react/index.js" 2.32 KB (0%)
import { useFragment } from "dist/react/index.js" (production) 2.27 KB (0%)
netlify[bot] commented 1 month ago

Deploy Preview for apollo-client-docs ready!

Name Link
Latest commit 5dd72ca93ed43c8a49a4522849d183a9e8c4caed
Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/66c8475cf696da0008de1103
Deploy Preview https://deploy-preview-12021--apollo-client-docs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

phryneas commented 1 month ago

Bisecting with

import gql from "graphql-tag";
import { ApolloClient, InMemoryCache } from "../../../core";
import { useQuery, ApolloProvider } from "../../../react";
import { renderHook } from "@testing-library/react";
import * as React from "react";

it("flips to `loading: true`, then `loading: false` on an empty cache when using `cache-only`", async () => {
  const query = gql`
    query {
      hello
    }
  `;
  const client = new ApolloClient({ cache: new InMemoryCache() });

  const renders = [];
  renderHook(
    () => renders.push(useQuery(query, { fetchPolicy: "cache-only" })),
    {
      wrapper: ({ children }) => (
        <ApolloProvider client={client}>{children}</ApolloProvider>
      ),
    }
  );
  await new Promise((resolve) => setTimeout(resolve, 1000));
  expect(renders[0].loading).toBe(true);
  expect(renders[0].data).toBe(undefined);
  expect(renders[1].loading).toBe(false);
  expect(renders[1].data).toBe(undefined);
});
phryneas commented 1 month ago

Okay, this has been the behaviour since 3.0.0: https://codesandbox.io/p/sandbox/ac-12021-nk7265

jpm4 commented 1 month ago

Thanks for digging into this!

If you do decide that loading: true is correct here, what about the case where we do get a cache hit? Should it still be loading: true very briefly?

phryneas commented 1 month ago

Hey @jpm4,

a short update for this: This is something we'd like to change, but it has been around for the whole 3.x series, so we will postpone it until 4.x. As for your question: It will depend on the implementation, but my current guess would be that it's a very short loading: true because we get those results asynchronously and the useQuery hook doesn't use suspense. That said, it's very likely that React's render batching would kick in here and the loading: true part would never actually be rendered out to the user.