Union's resolveType doesn't know about __typename

[ThisIsMissEm]

I'm currently trying to build a graphql server that uses the Result Union's pattern described by @sachee, whereby fields that may error in normal operation when resolving them return Union type of something like union PostResult = NotFound | Post

There's a video describing this pattern here: https://www.youtube.com/watch?v=GYBhHUGR1ZY

However, if you have:

export const PostResult = unionType({
  name: 'PostResult',
  description:
    'A Post, or NotFound if the post was not found',
  definition(t) {
    t.members('Post', 'NotFound');
    t.resolveType(item => item.__typename);
  }
});

Then you get an error telling you that __typename isn't a property of item, even though your resolver for the field that uses this PostResult union can definitely return the __typename field.

export const Query = objectType({
  name: 'Query',
  definition(t) {
    t.field('post', {
      type: PostResult,
      args: {
        id: idArg({ required: true })
      },
      resolve(root, { id }) {
        try {
          const post = await Posts.findById(id)
          return { __typename: "Post", ...post }
        } catch(err) {
          return { __typename: "NotFound", message: `Could not find post with ID: ${id}` }
        }
    });
  }
});

I think the type assigned to the callback argument in t.resolveType should have the __typename property available.

For now, I'm able to work-around this by using item['__typename'] but that's a pretty ugly hack.

[ThisIsMissEm]

Looks like it's possible to get this working by modifying typegenHelpers.d.ts to have:

export declare type ResultValue<
  TypeName extends string,
  FieldName extends string
> = GetGen3<'fieldTypes', TypeName, FieldName> & { __typename: string };

I've no idea how to actually get this as a proper fix though. Well, that was just part of it, to be able to correctly return __typename from a resolver for a union.

[tgriesser]

This is indeed an oversight and something I recently also realized/encountered as well when trying to build some union types using the same pattern from that talk.

Will look to get a fix in for this soon!

[jasonkuhrt]

I think the type assigned to the callback argument in t.resolveType should have the __typename property available.

Agree. We will need to make this invariant safe though. For example, making the resolver for a union-typed field be forced to include __typeName in the returned data. I expect more details to sort out than this.

[jasonkuhrt]

This feels like a bug to me, more than a feature.

[ThisIsMissEm]

For example, making the resolver for a union-typed field be forced to include __typeName in the returned data

This makes sense to me.

[tgriesser]

Started thinking more about this, and there's a few things to address here.

First, I believe the messaging / behavior of the current warning message is incorrect:

... or t.resolveType(() => null) if you don't want or need to implement.

You should always implement this somehow, () => null is useless for union types.

The question is really whether you want a custom resolve or whether you just want the default behavior, as @ThisIsMissEm is re-implementing by hand with the item => item.__typename.

If you are in-fact providing the __typename, then you can use the default one. As such, I was planning on changing the messaging/behavior to:

... or t.resolveType() if you want to use GraphQL's defaultTypeResolver and hide this warning.

So essentially if you call t.resolveType() with no arguments, it'll use defaultTypeResolver with behavior detailed here.

The tricky thing with typings here is that if you are using the default execution algorithm, __typename should be required for member types, but if you have a custom one then we shouldn't necessarily require __typename be specified, since that's the point of defining the custom resolveType.

@ThisIsMissEm does this sound like a reasonable approach/change to the issue you describe?

I was thinking of adding the change in the messaging / ability to call t.resolveType() for the soon to be released 0.12.0 and then address the __typename as part of the refactor of type generation in the next release.

[ThisIsMissEm]

So all unions will have to be written as:

export const PostResult = unionType({
  name: 'PostResult',
  description: 'A Post, or NotFound if the post was not found',
  definition(t) {
    t.members('Post', 'NotFound');
    t.resolveType();
  }
});

Wouldn't it be better to do:

export const PostResult = unionType({
  name: 'PostResult',
  description: 'A Post, or NotFound if the post was not found',
  definition(t) {
    t.members('Post', 'NotFound');
  }
});

Or even:

export const PostResult = unionType({
  name: 'PostResult',
  description:
    'A Post, or NotFound if the post was not found',
  members: ['Post', 'NotFound']
});

[tgriesser]

As I suggested, yes, though I suppose I was only looking at it as an improvement on the current alternative - rather than revisiting why it is how it is...

export const PostResult = unionType({
  name: 'PostResult',
  description:
    'A Post, or NotFound if the post was not found',
  members: ['Post', 'NotFound']
});

IIRC this API wasn't originally possible it was due to some issues w/ type completion/safety on object members not being correct, I think I left a comment about this as a reminder for a similar issue on interfaces, need to revisit if anything has changed here:

https://github.com/prisma-labs/nexus/blob/38fe229a1971f975f6ce60c8c3a6208856d72938/src/definitions/interfaceType.ts#L17-L22

It looks like the PR referenced there got merged, so maybe it is supported now? Will give it a shot and see if we can adjust/simplify the API here.

I guess we could also make the assumption that if resolveType isn't defined, __typename would be required on any possible union members?

[zingerj]

Just following up on this @tgriesser. What is the behavior today? Should we still be using t.resolveType(item => item.__typename); for now?

[YElyousfi]

Just following up on this @tgriesser. What is the behavior today? Should we still be using t.resolveType(item => item.__typename); for now?

I'd also like to follow up on this. What is the recommended approach?

[phillip055]

+1

[extjbhlego]

Awesome @jasonkuhrt - How long are your sprints? Excited to see this getting worked on! 🙌

[jasonkuhrt]

@jesperhalborglego two weeks. Note this issue is considered a stretch goal of our sprint :)

[jasonkuhrt]

The tricky thing with typings here is that if you are using the default execution algorithm, __typename should be required for member types, but if you have a custom one then we shouldn't necessarily require __typename be specified, since that's the point of defining the custom resolveType.

Thinking about this.

• Restating the problem: GraphQL Clients, tools, and fragments use __typename field on members of a returned union type to reconstruct what types are being dealt with at runtime. This is a special spec-compliant field that does not show up in the GraphQL SDL Schema. Nexus Schema allows developers to provide this data one of two ways:

  1. In the resolver for the field returning a union type all the objects being returned are "marked"/"branded"/whatever with a property __typename whose string value matches exactly the name of the GraphQL object type that the data conforms to (graphql.js raises standard error upon non-conformance).
  2. In the union type definition a special method is implemented called t.resolveType. It runs for each object being returned for that union at runtime. Each run must result in the string value that matches exactly the name of the GraphQL object type that the data conforms too.

• If a union type definition has implemented t.resolveType then the backing types of members of the union in the context of the union field resolve do not require __typename.

• If the backing data typings for any field returning that union type are such that members of that union provide a __typename then no t.resolveType is required at the union type definition level.

• If neither of the above is true then we're left with trying to figure out which one the developer should satisfy. Nexus cannot know. The best it can do is raise errors on both sides statically, and dynamically raise a useful runtime error during development. Unfortunately, statically, there will be no easy way to explain that two static errors are mutually exclusive and only ONE must be resolved by the developer. Docs, and runtime feedback, will be our best way to support beginners. Veterans will be fine, though may find this lack of precise feedback "annoying".

• Note: Even if a developer provides t.resolveType they may still augment the backing data types as they wish, of course. If they augment it such that they state the backing type of an object will always have __typename. In this case it just means that the developer will have access to __typename in their t.resolveType implementation.

[jasonkuhrt]

Here is a revised spec following discussion with @Weakky

1. When an object type is just a member of an unexposed union nothing special has to happen

2. When an object type is a member of a union and the union type is exposed directly or indirectly through a root field then the user will have access to three kinds of static feedback:

   1. A resolveType static error
   2. An isTypeOf static error
   3. A resolver (the one for the field whose return type is the union) return type static error

3. A breakdown of the three types

   1. resolveType error is on the union type definition config, a new field called resolveType. It will replace t.resolveType API. The reason we cannot use t.resolveType is that there is no way in TS to require, statically, for a function to be called by the user.

      unionType({
        // TS ERROR: Missing property `resolveType` ...
        name: 'ABC',
        definition(t) {
          t.members('A', 'B')
        },
      })

   2. isTypeOf error is on the object type definition config, a new field called isTypeOf. There is no t.isTypeOf API to replace. The reason we cannot use t.isTypeOf is for the same reason given for not using t.resolveType.

      objectType({
        // TS ERROR: Missing property `isTypeOf` ...
        name: 'A',
        definition(t) {
          // ...
        },
      })

      objectType({
        // TS ERROR: Missing `isTypeOf` ...
        name: 'B',
        definition(t) {
          // ...
        },
      })

   3. Resolver return type error is for every instance of a field whose type is that union.

      queryType({
        definition(t) {
          t.field('abc', {
            type: 'ABC',
            resolve() {
              // TS ERROR: Missing property `__typename: "A" | "B" | "C"` ...
              return ...
            }
          })
          t.list.field('abcs', {
            type: 'ABC',
            resolve() {
              // TS ERROR: Missing property on each object `__typename: "A" | "B" | "C"` ...
              return ...
            }
          })
        },
      })

4. There are problems with the static errors as a means of feedback. The three types of error feedback happen at once, solving any category of them is all that is needed, but this is not obvious. It will not even be obvious necessarily to parse the categories of errors in one's own mind. TS will just display all the static errors in their raw unrelated and ungrouped form.

   For this reason there will be a configuration option to enable categories of feedback:

   makeSchema({
     checks: {
       unions: {
         isTypeOf?: boolean // default true
         resolveType?: boolean // default false
         backingType?: boolean // default false
       }
     }
   })

   By default Nexus will only give the isTypeOf static feedback. However for teams working with different standards, they may change to enable a different set of the union checks.

5. To aid the feedback, there will be rich runtime feedback that gives more in-depth explanation about what and where the issue is, and how and where to fix it.

6. jsdoc will be used extensively to help guide the developer

7. website doc will be extensive too

References

• https://github.com/graphql/graphql-js/blob/master/src/execution/execute.js#L1157-L1195
• What is the purpose of resolveType in Interface? graphql/graphql-js#876 (comment)
• Infer contextual types from generic return types microsoft/TypeScript#29478

[extjbhlego]

Very nice analysis and writeup, and the proposed solution is great in my opinion 👏

[tgriesser]

One of Nexus' opinions originally was not to attempt to support isTypeOf because it always felt clunky to have an exhaustive check on possible member types before failure, rather than always defining the resolveType as part of the interface / union type.

At least in my experience, it felt like having two ways to do the same thing felt odd / distracting, and you were better off just implementing resolveType in almost all cases - I do wonder if there are valid reasons you'd prefer / need isTypeOf over resolveType though.

resolveType error is on the union type definition config, a new field called resolveType. It will replace t.resolveType API. The reason we cannot use t.resolveType is that there is no way in TS to require, statically, for a function to be called by the user.

This sounds good to me. I am trying to remember if there was a typings reason for having t.resolveType rather than having it as a config member on union / interface types.

By default Nexus will only give the isTypeOf static feedback. However for teams working with different standards, they may change to enable a different set of the union checks.

Interested to hear thoughts re: feedback above on the value of implementing isTypeOf in general. Hasn't seemed to be anything folks have complained about missing from graphql-js.

[jasonkuhrt]

Thanks for the feedback @tgriesser!

Interested to hear thoughts re: feedback above on the value of implementing isTypeOf in general. Hasn't seemed to be anything folks have complained about missing from graphql-js.

I think the main point @Weakky raised was that isTypeOf is more modular.

If we all agree that it is OK for Nexus Schema to technically support all techniques then I see this discussion about what the best default is. Do you all see it the same way?

I don't feel strongly personally about the central resolveType approach or the distributed isTypeOf approach. I only feel strongly that the default checks should be setup such that only one check is enabled.

@Weakky thoughts?

[Weakky]

At least in my experience, it felt like having two ways to do the same thing felt odd / distracting, and you were better off just implementing resolveType in almost all cases - I do wonder if there are valid reasons you'd prefer / need isTypeOf over resolveType though.

So most of my reasoning for adding isTypeOf to the API came from that comment from @leebyron 👇:

[...] However because Interfaces are often broadly used, maintainability of a resolveType() method can become difficult if there are very many implementing Object types for that Interface. Ideally when adding a new Object type that implements an Interface, you don't need to edit the code for the Interface type as well. The isTypeOf() method allows you to do this. This is possible because resolveType() has a default implementation which looks at all implementing types, and calls isTypeOf() on each until one returns true.

While everybody seems to be using resolveType, isTypeOf looks way more modular and makes the evolvability of the API much easier. It feels to me that people aren't using isTypeOf because they don't know about it, but I might be totally wrong.

Here's my humble question to you @tgriesser: Why are we placing the logic to discriminate different types in a single place when we have the opportunity to cleanly couple that to the types themselves? (Which enables a type to be used in any interface/union type, multiple times, without refactoring anything)

With isTypeOf

eg (pseudo-code):

objectType({
  name: 'A',
  implements: 'AB',
  isTypeOf: (val: A | B |  C) => isA(val)
})

objectType({
  name: 'B',
  implements: 'AB',
  isTypeOf: (val: A | B | C) => isB(val)
})

objectType({
  name: 'C',
  isTypeOf: (val: A | B | C) => isC(val)
})

interfaceType({
  name: 'AB', // no need for a custom resolveType
})

uniontType({
  name: 'AorC',
  members: ['A', 'C'] // no need for a custom resolveType
})

👆A common pattern with isTypeOf, if you're using an ORM that wraps your data in classes/entities/models (like TypeORM), is to use instanceof to easily discriminate your Objects.

eg (pseudo-code):

import { UserEntity } from './entities'

objectType({
  name: 'User',
  isTypeOf: (val) => val instanceof UserEntity
})

With resolveType

With resolveType, you need a custom implementation for every interface/union you have. If they overlap, you might also need to think about how you're gonna share the discrimination logic across your different interfaces/unions

eg (pseudo-code):

objectType({
  name: 'A',
  implements: 'AB'
})

objectType({
  name: 'B',
  implements: 'AB'
})

objectType({
  name: 'C',
})

interfaceType({
  name: 'AB',
  resolveType(val: A | B) {
    if (isA(val)) {
      return 'A'
    }
    if (isB(val)) {
      return 'B'
    }

    assertNoFallthrough(val)
  }
})

uniontType({
  name: 'AorC',
  members: ['A', 'C'],
  resolveType(val: A | C) {
    if (isA(val)) {
      return 'A'
    }
    if (isC(val)) {
      return 'C'
    }

    assertNoFallthrough(val)
  }
})


function assertNoFallthrough(x: never) {
  throw new Error('Case not handled')
}

Conclusion

Given these two examples, the isTypeOf approach feels way cleaner to me. Not only does it reduce code duplication, but it also makes handling big interfaces/unions super easy. Imagine multiple unions of 10+ members with overlaps between each other. I wouldn't want to have to deal with big switch cases/if cascades in every one of my union.

There is one case where resolveType is better, and that's if you have a discriminating property that has the same value as your object type name, but that property isn't called __typename (otherwise you can use the default implementation of resolveType). In this case, your data is somehow already discriminated anyway, so there's no need to use isTypeOf. A custom resolveType implementation is preferred:

eg (pseudo-code):

interface A {
  type: 'A'
}

interface B {
  type: 'B'
}

resolveType(val: A | B) {
  return val.type
}

That being said, I want to acknowledge the fact that I've never really implemented interfaces/unions at scale, so none of what I said above are strong statements, just observations, and I might be missing something.

Let me know what y'all think 🙌

[jasonkuhrt]

Thanks for the great write up @Weakky. I'll wear my devil's advocate hat and share some thoughts. Also again to be clear I think we're merely discussing what the default enabled checks should be.

1. About discriminating you mentioned:

I misunderstood :)

If they overlap, you might also need to think about how you're gonna share the discrimination logic across your different interfaces/unions

I believe this problem is shared by both techniques. Either a central function runs that returns a type name or a little internal loop runs over all the isTypeOf defs doing basically the same. I think this problem is actually harder in the distributed case. At least with centralization the overlapping might become more obvious. But anyways the real way to get safety here is probably via testing or using an ORM that provides type dispatching guarantees (e.g. like you showed with typeORM).

2. I think one advantage of resolveType is that it is more incremental in the learning curve––sort of. What I mean is that when you begin introducing unions or interfaces you'll be learning what that means for the first time. When working with resolveType you get to solve the problem in the same place where you're learning about implementing union/interface types. This is a minor point, though.

---

Overall I am leaning towards @Weakky's point of view here, that the default strategy should be isTypeOf. I think this default would be optimized for larger and more serious projects/patterns. I think optimizing for that is consistent with another default we have in Nexus: nullability by default on outputs. That isn't convenient, but it is safer and thus optimized for serious work.

--edit

More discussion with @Weakky and I feel pretty strong now that isTypeOf is superior. The n union membership and maybe-1 implements interface makes a solid case for the code-reuse benefit of isTypeOf.

[Sytten]

I feel both approach should be allowed for maximum flexibility.
I quickly read through but how do you ensure that isTypeOf is present on union members? (Without imposing it on all models)
I think the initial point of having an easy way to pass down the __typename and using that as the first source of truth is still valid as this is what I do for my projects.

[jasonkuhrt]

@Sytten The docs should answer any questions you have. If not we need better docs. Let us know! https://github.com/graphql-nexus/schema/pull/602/files#diff-2e34d9dffe181b23833ccc9ee55d87eb6b175066626b0e21feeafd145d94efa5

[Sytten]

Doc seems good! I am glad multiple strategies are allowed.