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

useFragment fails to get data from cache for an Interface, even when possibleTypes is setup correctly #11583

Closed
Stephen2 opened this issue Feb 9, 2024 · 6 comments

Comments

@Stephen2
Copy link
Contributor

Stephen2 commented Feb 9, 2024

Issue Description

I'm sorry in advance, there's a lot of complexity and I'm kinda new to Apollo. I don't have a minimum repro for you, but hope this minimum explanation & screenshots is enough.

  • Schema has an interface CandidateSubmissionData with a type type ApplyCandidateSubmission implements CandidateSubmissionData

I believe I've informed Apollo client cache of this correctly:

    cache.policies.addPossibleTypes({
        CandidateSubmissionData: [
            'ApplyCandidateSubmission',
        ],
    });
  • Key field for any CandidateSubmissionData is submissionUuid: ID!

I believe I've informed Apollo client cache of this correctly:

cache.policies.addTypePolicies({
        CandidateSubmissionData: {
            keyFields: ['submissionUuid'],
         }
});

Fragment defined like:

fragment MyFragment on CandidateSubmissionData {
  submissionUuid
  ... other fields here ...
}

Fragment data accessed like:

    const { data, complete } = useFragment({
        fragment,
        fragmentName,
        from: {
            __typename: 'CandidateSubmissionData', <--- NOTE: this is the interface name
            submissionUuid,
        },
    });

So I thought this would be enough to get everything working, but here's where I think the bug comes in.

I can see the object made it's way to normalized cache, by using DevTools:
image
However, I was personally expecting it to store as the Interface name CandidateSubmissionData but OK, maybe I just don't understand how possibleTypes work.

But my fragment is not fulfilled, complete === false.

If I modify the fragment to:

    const { data, complete } = useFragment({
        fragment,
        fragmentName,
        from: {
            __typename: 'ApplyCandidateSubmission', <--- NOTE: this is the Type name
            submissionUuid,
        },
    });

Then complete === true and my component has it's data from the cache.


The final straw that made me think this might just be a bug, and not me missing something, is playing with cache.identify:

    const a = cache.identify({
        __typename: 'CandidateSubmissionData',
        submissionUuid,
    });

    const b = cache.identify({
        __typename: 'ApplyCandidateSubmission',
        submissionUuid,
    });

    console.log('nameplate', {
        a,
        b,
    });
image got 'em both ways. So that makes me think `useFragment` should be able to get the data both ways, but it doesn't :(

Workaround my colleague figured out, with my comment above it:

        CandidateSubmissionData: {
            // This doesn't make sense to me, that I'm needing to do this, but without it, despite adding the possibleTypes correctly
            // the `useFragment.from.__typename === CanidateSubmissionData` calls don't return the object, despite being stored
            // in the cache, _and_ cache.identify() working with either __typename (Interface or implmeneting Type)
            // I've raised a Github issue to see if Apollo team considers this a bug.
            keyFields: (object) => {
                const { submissionUuid } = object;

                if (typeof submissionUuid !== 'string') {
                    return false;
                }

                const key = {
                    submissionUuid,
                };

                return `CandidateSubmissionData:${JSON.stringify(key)}`;
            },
        },

Thanks for reading my essay, and I look forward to your help with understanding this 🙏

Link to Reproduction

n/a

Reproduction Steps

No response

@apollo/client version

3.8.6

@jerelmiller
Copy link
Member

jerelmiller commented Feb 9, 2024

Hey @Stephen2 👋

Thanks for the question! I'll do my best to explain the behavior you're seeing here.

I just don't understand how possibleTypes work.

possibleTypes is a way for you to explain the subtype/supertype relationship between types in your GraphQL schema. This is necessary for queries that return in-line fragments to work.

What's neat about possibleTypes is that it allows you to share type policies among several subtypes by defining a type on your supertype, which you've done here with keyFields on CandidateSubmissionData. This saves you from needing to copy that keyFields configuration over to every subtype that would implement that interface.

I was personally expecting it to store as the Interface name CandidateSubmissionData

You actually wouldn't want us to do this! Interfaces do not guarantee uniqueness among their subtypes, so if we were to store data using the interface type, there is a possibility we might clobber data together in ways that don't make sense. Take the following schema:

interface Character {
  id: ID!
  name: String!
}

type Jedi implements Character {
  id: ID!
  name: String!
  lightsaberColor: String
}

type Droid implements Character {
  id: ID!
  name: String!
  primaryFunction: String
}

And this query:

query {
  characters {
    id
    name
    ... on Jedi {
      lightsaberColor
    }
    ...on Droid {
      primaryFunction
    }
  }
}

Since that interface doesn't guarantee uniqueness, its possible we get 2 different characters with the same id, which is perfectly valid:

{
  data: {
    characters: [
      {
        __typename: 'Jedi', 
        id: 1, 
        name: 'Luke Skywalker',
        lightsaberColor: 'green'
      },
      {
        __typename: 'Droid', 
        id: 1, 
        name: 'R2-D2',
        primaryFunction: 'Astromech'
      }
    ]
  }
}

If we were to write this to the same cache entry, you'll get a mix of data that wasn't meant to be combined:

{
  'Character:1': {
    __typename: 'Character',
    id: 1,
    // Oh no, we've lost Luke!
    name: 'R2-D2',
    // Uh-oh, we have both Jedi and Droid data here!
    lightsaberColor: 'green', 
    primaryFunction: 'Astromech',
  }
}

Instead, we write these using their subtypes to ensure the data is isolated from each other.

This also goes for reading data out of the cache. It's impossible to determine which is the "correct" entity to read out of the cache if you provide the interface type as the __typename:

useFragment({ 
  // Should we return Jedi:1 or Droid:1 here?
  from: { __typename: 'Character', id: 1 } 
})

For this reason, we require you to pass the subtype to useFragment so that we can provide you with the right entity.

The final straw that made me think this might just be a bug, and not me missing something, is playing with cache.identify:

cache.identify is just a helper method that takes an object with __typename and key fields, looks up the associated type policy for that type, and gives you back a string cache ID. It's not smart enough though to detect if that __typename is itself a supertype, so it just returns that name (though it is smart enough to know if that __typename is a subtype). We probably could warn here since you'd never want to use the interface cache id, but this is not something it does today.

Workaround my colleague figured out

I'd avoid doing this because as the above example demonstrates, storing the data under the interface type is dangerous and you might end up mixing data that should be isolated from each other and overwriting common field names between subtypes. Instead just be sure to use the subtypes in useFragment to ensure you're getting the data you expect.

I hope this helps and sheds some light on why this works that way!

@Stephen2
Copy link
Contributor Author

Stephen2 commented Feb 10, 2024

Wow, thank you so much for taking the time for these excellent explanations!

we require you to pass the subtype to useFragment
Sounds like it might not be possible for me to have a component call useFragment when the component is agnostic about the implementing Type, and just wants to operate on the Interface?

For the concrete example, using your examples above:

interface Character {
  id: ID!
  name: String!
}

Say a component that shows the name for either Jedi or Droid wouldn't be possible with just 1 component and 1 useFragment call.

That's a bit of a blow. Let me know.

Thinking out loud - If not possible to useFragment, I suppose I can use prop drilling to get the fragment into the component.


In my case, I do know that the all the Types implementing Interface must have a globally unique submissionUuid so our hack is "safe", but I do take your lesson loud & clear that this shouldn't be how to do things in Apollo

@jerelmiller
Copy link
Member

Sounds like it might not be possible for me to have a component call useFragment when the component is agnostic about the implementing Type, and just wants to operate on the Interface?

I probably should have clarified, this is only specific to the value you pass to from. You are absolutely able to use a GraphQL fragment that works on the interface type! You just can't pass a from that contains a __typename that is the interface type itself.

This is perfectly valid however:

const CHARACTER_FRAGMENT = gql`
  # We can use interface types in our fragment document just fine!
  fragment CharacterFragment on Character {
    id
    name
  }
`

interface CharacterProps {
  id: string;
  type: 'Droid' | 'Jedi';
}

function Character({ id, type }: CharacterTypes) {
  const { data } = useFragment({ 
    fragment: CHARACTER_FRAGMENT,
    // This just needs to contain a `__typename` that is either "Droid" or "Jedi", 
    // but not "Character"
    from: { __typename: type, id }
  });

  return <div>{data.name}</div>
}

I don't fully understand how you've got your app setup, but the only thing you'd need to make sure to pass to this component is the subtype name, but it will be able to handle both interface types correctly.

Does that help?

@Stephen2
Copy link
Contributor Author

Yeah it helps a lot.

I just realised from your explanation that I could pass in from the parent that runs the query, request and pass down the __typename into the component, and use that for the from just like you did there.

I'll have to think on this all, because if I'm prop drilling __typename down anyway, maybe it just makes sense to prop drill the fragmentData directly.

I think I'm going to close this now, I think I understand all your lessons 🙏 what a lucky day, for me to have answers from someone so quick to respond & knowledgeable!! cheers mate

Copy link
Contributor

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants