Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phryneas
Copy link
Member

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.

Copy link

changeset-bot bot commented Aug 23, 2024

⚠️ No Changeset found

Latest commit: 5dd72ca

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

Copy link
Contributor

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%)

Copy link

netlify bot commented Aug 23, 2024

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit 5dd72ca
🔍 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
Copy link
Member Author

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
Copy link
Member Author

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

@phryneas phryneas marked this pull request as draft August 23, 2024 09:09
@jpm4
Copy link

jpm4 commented Aug 23, 2024

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
Copy link
Member Author

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.

@jerelmiller jerelmiller added this to the Release 4.0 milestone Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants