Skip to content

Conversation

@ryo-manba
Copy link

@ryo-manba ryo-manba commented Feb 1, 2025

Closes #12003
Closes #11600

What

Fixes a bug where watchFragment and useFragment returned { complete: true } even when the from object could not be identified (i.e. cache.identify() returned undefined).

Now, in such cases, the result correctly includes complete: false and missing field info.

Why

Previously, non-identifiable objects skipped fragment field checks, leaving missing empty.
This led to a false assumption that data was complete, causing confusing behavior in apps using partial data or suspense.

We now detect this edge case early in InMemoryCache.diff() and return the correct complete: false result.


This was created from the branch adds the failing test.
It might be better to merge that one first.

@apollo-cla
Copy link

@ryo-manba: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@netlify
Copy link

netlify bot commented Feb 1, 2025

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 6dc4c34

@changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2025

🦋 Changeset detected

Latest commit: 739b62f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

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

@svc-apollo-docs
Copy link

svc-apollo-docs commented Feb 1, 2025

❌ Docs preview failed

The preview failed to build.

Build ID: ba84bc7fd4e44a0393adbc41

Errors

react/api/react/components

  • Cannot find module '@microsoft/tsdoc/schemas/tsdoc.schema.json' from '/var/task/node_modules/.pnpm/@microsoft[email protected]/node_modules/@microsoft/tsdoc-config/lib'
    react/api/react/hooks

  • Cannot find module '@microsoft/tsdoc/schemas/tsdoc.schema.json' from '/var/task/node_modules/.pnpm/@microsoft[email protected]/node_modules/@microsoft/tsdoc-config/lib'
    react/data/queries

  • Cannot find module '@microsoft/tsdoc/schemas/tsdoc.schema.json' from '/var/task/node_modules/.pnpm/@microsoft[email protected]/node_modules/@microsoft/tsdoc-config/lib'
    react/errors

  • Transformer not found for component "DisplayClientError"
    react/local-state/managing-state-with-field-policies

  • Could not parse expression with acorn

@ryo-manba ryo-manba marked this pull request as ready for review February 1, 2025 03:36
@jerelmiller
Copy link
Member

Hey @ryo-manba 👋

Thanks for your patience!

I'd actually like to dig a layer deeper and see if we can fix the issue at the root. It looks like anything having to do with a Cache.DiffResult is affected by this issue. Since watchFragment calls cache.watch() under the hood, the issue is somewhere inside the result returned from that.

I can also reproduce the incorrect result using cache.diff with the following test:

test.only("reports diffs incorrectly", async () => {
  const cache = new InMemoryCache();

  console.log(
    cache.diff({
      query: cache["getFragmentDoc"](gql`
        fragment FooFragment on Foo {
          foo
        }
      `),
      id: cache.identify({}),
      returnPartialData: true,
      optimistic: true,
    })
  );

  await new Promise((res) => {
    const w = cache.watch({
      query: cache["getFragmentDoc"](gql`
        fragment FooFragment on Foo {
          foo
        }
      `),
      id: cache.identify({}),
      returnPartialData: true,
      immediate: true,
      optimistic: true,
      callback: (diff) => {
        console.log("callback", diff);

        res(void 0);
      },
    });
  });
});

If you paste and run this, you'll notice that cache.diff also returns a complete: true, even though it has no data.

From what I can tell, that getFragmentDoc thing is what triggers the issue. If I replace the document with a plain query, the data is properly reported as complete: false.

Let's see if we can fix it in the cache itself. Is this something you'd like to look into further?

@ryo-manba
Copy link
Author

Thank you for the information!
I'll dig into it a bit more.

@ryo-manba ryo-manba marked this pull request as draft July 13, 2025 06:29
@ryo-manba ryo-manba changed the title Fix watchFragment reports complete=false when from object is not identifiable Fix watchFragment and useFragment incorrectly reporting complete: true for non-identifiable objects Jul 13, 2025
@ryo-manba ryo-manba marked this pull request as ready for review July 13, 2025 09:53
@ryo-manba
Copy link
Author

@jerelmiller
Sorry for the delay! Implementation is complete and ready for review.
Thanks for your patience!

@jerelmiller
Copy link
Member

Hey @ryo-manba 👋

Thanks a bunch! Let me put this up for discussion on our next team meeting. We are very close to releasing versions 3.14 and 4.0 very soon so we'll need to figure out where this change fits best. Thanks again for following through!

@jerelmiller
Copy link
Member

Hey @ryo-manba! Thanks for your patience!

We'd like to get this fix in 4.0, though after looking at this, we'd like to fix the cache.diff method so that it can work with fragments directly without having to create a query document out of it. That would make it much easier to detect whether we need an id or can use ROOT_QUERY as a fallback for actual queries.

I'll push some changes on top of this PR in the coming days to get that fix in place (I'm currently focused on writing some documentation for 4.0 before I can get to this).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants