Apollo-client: Schema knowledge needed for fragmentMatcher

[fubhy]

Currently, when working with Interfaces/Unions, the fragmentMatcher runs into a problem when trying to detect if a field value is missing (e.g. because the previous query simply didn't include the field) or if the field simply doesn't exist on the type at all. In the latter case, the fragmentMatcher should return false. However, since we don't currently have any knowledge of the types and fields in the schema, we always return true here as a band-aid solution.

https://github.com/apollographql/apollo-client/blob/master/src/data/readFromStore.ts#L164

Intended outcome:
Return false when a field does not exist on a fragment's type.

Actual outcome:
Currently, we always return true because we simply don't have any knowledge of the types and fields

How to reproduce the issue:
Run a query with two fragments with different subselections on a union/interface field. The query will always be re-executed. This is especially annoying when doing SSR. In this case, the client re-runs the queries although it wouldn't be necessary.

Propose solution
Add limited knowledge about the schema (available fields on all types) to apollo-client and change the fragmentMatcher accordingly.

[stubailo]

There are two options here:

1. Load the list of all interfaces/unions and the implementing types up front on startup, and use this info in fragment matcher
2. Load the data for each query inside the request. This avoids having to do a chunk of work up front.

Conveniently enough, the GraphQL query language has the information we need to convert this:

{
  search {
    ... on Human {
      firstName
    }
    ... on Droid {
      modelNumber
    }
  }
}

Into:

{
  search {
    ... on Human {
      firstName
    }
    ... on Droid {
      modelNumber
    }
  }
  __Human: __type(name: "Human") {
    possibleTypes {
      name
    }  
  }
  __Droid: __type(name: "Droid") {
    possibleTypes {
      name
    }  
  }
}

That introspection gives us all of the information we need to pass into fragmentMatcher.

[calebmer]

I still don’t think we need a fragmentMatcher with a better design of the store as I have discussed with @helfer. If we add __typenames to paths in the store, so for example ROOT_QUERY.search.foo becomes ROOT_QUERY.search:Human.foo:Foo or ROOT_QUERY.search:Driud.foo:Foo I don’t see how this could be a problem.

[fubhy]

Clever idea. I think I understand what you are trying to do but I believe that this would still potentially leave us with one remaining issue in some cases. Note: I did not have a close look at the read/write logic in the store yet so I might be missing something here ...

You can write the following query in different ways:

{
  search {
    ... on Human {
      bloodType
      firstName
      universalGalacticPassportNumber
    }

    ... on Droid {
      universalGalacticPassportNumber
      modelNumber
    }
  }

{
  search {
    universalGalacticPassportNumber

    ... on CanBleedInterface {
       bloodType
    }

    ... on Human {
      firstName
    }

    ... on Droid {
      modelNumber
    }
  }

By the way: It's a valid query to ask for identically named fields on different inline fragments. Even if they actually refer to the exact same field of a common interface. Or: Identically named fields on different inline fragments that actually don't have a common interface... Woah.

Anyways, I believe the queries above show we actually can't use the __typename because then, interface based inline fragments would still cause the aforementioned issue. Because at query processing time we would then still not be able to reliably work out (e.g. in case of the second query) if ROOT_QUERY.search:Droid.bloodType or ROOT_QUERY.search:Human.bloodType actually exist or not.

Instead, we might want to use the inline fragment's name. This would require us to take the query structure into consideration during the WRITE operation instead of just using the structure of the response. But it would solve some of the aforementioned issues.

However, there might still be problems with that solution too...

Am I missing something here? As I said, I didn't dive deep into the read/write store logic yet.

[fubhy]

I submitted https://github.com/apollographql/apollo-client/pull/1360/files with the query transform I created (introspection at runtime). Obviously that is not ideal and we need to find a better solution.

[helfer]

A list of related issues that are likely to be fixed with the new fragment matcher:

1363 #1379 #1113 #857 #771 #1297 #1273 #961 #933

[helfer]

@fubhy any update on your progress? This is a pretty high priority feature for us, so I'd like to finish it this week. If you don't have time, you could just make a PR with what you have, and then I'll take it from there. 🙂 Either way, a quick answer would be appreciated.

[helfer]

@fubhy heads-up: I'll start working on this today.