Allow unions of (non-interface) Objects

[StevenLangbroek]

As discussed on spectrum, starting repro here.

[kitten]

So we are checking here whether we may have a UnionType: https://github.com/FormidableLabs/urql-exchange-graphcache/blob/6e0f016beadf224249cfb5d9362ca979b484fc24/src/ast/schemaPredicates.ts#L98

You can probably add a case here that checks:

schemaPredicates.isInterfaceOfType('Todo', 'NoTodosError');
schemaPredicates.isInterfaceOfType('NoTodosError', 'Todo')

But I expect our bug to be here: https://github.com/FormidableLabs/urql-exchange-graphcache/blob/6e0f016beadf224249cfb5d9362ca979b484fc24/src/ast/schemaPredicates.ts#L47

We're assuming that typeCondition (so ... on NoTodosError) to be an abstract type when the check fails, but for unions it's actually a regular object type, so we'd have to also check whether we just get the same ObjectType, so:

    const abstractType = this.schema.getType(typeCondition);
+    if (abstractType instanceof GraphQLObjectType) {
+      return abstractType === objectType;
+    }
    expectAbstractType(abstractType, typeCondition);
    const objectType = this.schema.getType(typename);
    expectObjectType(objectType, typename);

[StevenLangbroek]

So, now it makes a little bit more sense to me; this would be the result of

type Todo {

}

type NoTodosError {

}

union LatestTodoResult = Todo | NoTodosError

[kitten]

I suppose we may want to rename this method now to sth like isMatchingCondition

[StevenLangbroek]

I'll refrain from commenting further on what does and doesn't make sense until I've spent some more time on GraphQL primitives. The fact that it was counterintuitive to me probably has more to do with that than the actual API.

[StevenLangbroek]

To be honest, I can't really reason about how this test actually implies correctness. What's the failure path here?

[StevenLangbroek]

@kitten @JoviDeCroock is there anything left to do on this?

[JoviDeCroock]

We might have to rebase with master so the CI can run correctly

[StevenLangbroek]

Done :)

[StevenLangbroek]

Lol thanks codecov.

[kitten]

I think we're missing a test for this:

expect(
  schemaPredicates.isInterfaceOfType('Todo', 'NoTodosError')
).toBeFalsy();

Also is LatestTodoResult compared with NoTodosError / Todo legal? As in are you able to write this in GraphQL? Because I'm not sure

fragment _ on LatestTodoResult {
}

Where LatestTodoResult is obviously a union, hence a fragment shouldn't be able to match at all 🤔

[StevenLangbroek]

I think that should be ok, but should only yield __typename?

[StevenLangbroek]

Yep that's valid (sorry commented on wrong thread):

[JoviDeCroock]

Great work, thank you so much for taking your time to resolve this!