Closed ThisIsMissEm closed 3 years ago
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.
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!
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.
This feels like a bug to me, more than a feature.
For example, making the resolver for a union-typed field be forced to include __typeName in the returned data
This makes sense to me.
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.
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']
});
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:
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?
Just following up on this @tgriesser. What is the behavior today? Should we still be using t.resolveType(item => item.__typename);
for now?
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?
+1
Awesome @jasonkuhrt - How long are your sprints? Excited to see this getting worked on! π
@jesperhalborglego two weeks. Note this issue is considered a stretch goal of our sprint :)
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:
__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).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.
Here is a revised spec following discussion with @Weakky
When an object type is just a member of an unexposed union nothing special has to happen
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:
resolveType
static errorisTypeOf
static errorA breakdown of the three types
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')
},
})
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) {
// ...
},
})
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 ...
}
})
},
})
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.
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.
jsdoc will be used extensively to help guide the developer
website doc will be extensive too
Very nice analysis and writeup, and the proposed solution is great in my opinion π
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
.
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?
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)
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
})
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')
}
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 π
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.
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
.
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.
@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
Doc seems good! I am glad multiple strategies are allowed.
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:
Then you get an error telling you that
__typename
isn't a property ofitem
, even though your resolver for the field that uses this PostResult union can definitely return the__typename
field.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.