(graphcache)- readFragment targets the first fragment of a DocumentNode

[narkeeso]

Describe the bug

When using a fragment with nested fragments readFragment will return null if your nested fragment is ordered a certain way.

Non-working Example:

// This doesn't work, observe that the nested Note fragment is defined first before using it
gql`
  fragment Note on note {
    id
  } 

  fragment User on user {
    id
    notes {
      ...Note
    }
  }
`

Working Examples (workarounds):

// Moving the Note fragment to the bottom strangely works, it's probably being hoisted?
gql`
  fragment User on user {
    id
    notes {
      ...Note
    }

  fragment Note on note {
    id
  } 
`

// or don't use nested fragments
gql`
  fragment User on user {
    id
    notes {
      id
    }
  }
`

I've attached a CodeSandbox with the reproduction. Uncomment out the various fragments at the top of the sandbox to observe how readFragment works with various fragment configurations. The result is printed to console.

Reproduction

https://codesandbox.io/s/urql-nested-fragments-bug-7ez7tr?file=/src/App.tsx

Urql version

2.2.3

Validations

• [X] I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
• [X] Read the docs.
• [X] Follow our Code of Conduct

[JoviDeCroock]

This kind off feels like intended behavior, we take the first fragment we can find because it's a heuristic to see what base-type you are looking for. What assumption would you expect here? Us iterating over all fragments to find the one who doesn't have any spreads, this could also be an unused one in that case though, we could ask to specify the type the user is looking for but this might give issues with interfaces/unions 😅

[narkeeso]

The behavior feels inconsistent and slightly unintuitive. When using it in a query it works as expected. Declaring the fragment before usage feels like the correct thing to do since expecting your fragment to get hoisted for use is less intuitive.

If this is how readFragment is intending to behave we should probably add a note in the docs at least, I lost a good chunk of time figuring this out. Although I do believe it should support the originally intended method of declaring your nested fragments first.

We also don’t typically write the fragment as a literal, we usually reference a constant that’s being used in an actual query so that we can make sure the cache understands this dependency and re-writing the fragments to be compatible with readFragment is not ideal.

[JoviDeCroock]

I mean, the alternative is to allow users to specify the name of the fragment you want to leverage currently we follow the presedence of first defined is the one you need. I haven't seen many people use their actual queries in the cache updates because of code-splitting/.... but yes generally one of the solutions here would be to specify the typeCondition or fragmentName for the data you want to access

[narkeeso]

Think it would be great to be able to target with fragmentName as an optional parameter.