-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow to override default type resolver #1332
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I guess @leebyron should approve.
src/execution/execute.js
Outdated
const resolveTypeFn = returnType.resolveType || exeContext.typeResolver; | ||
const contextValue = exeContext.contextValue; | ||
const typeInfo = { ...info, valueType: returnType, valuePath: path }; | ||
const runtimeType = resolveTypeFn(result, contextValue, typeInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid creating a new object for every field resolution?
Perhaps these lines should be:
const runtimeType = returnType.resolveType
? returnType.resolveType(result, exeContext.contextValue, info)
: exeContext.typeResolver
? exeContext.typeResolver(returnType, result, exeContext.contextValue, info)
: defaultResolveTypeFn(returnType, result, exeContext.contextValue, info)
Also, path
is already in info
, so I don't think we need to add it again as valuePath
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to this type signature - I'm not sure how valuable returnType
will be for a default resolver - if it deserves to be the first argument or not. I imagine result
would still be the most useful.
Another option might be to just provide returnType
as another argument after info to all type resolvers - not just default ones?
e3632d6
to
daea78b
Compare
@leebyron Done 👍 |
daea78b
to
0a1306b
Compare
0a1306b
to
f592b89
Compare
f592b89
to
9f605b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure that I get this change correctly: You allow to specify a custom type resolved, that if specified replaces the default resolver. Does that sound right?
Overall I am not a fan that we need to change types in so many places for this to work, but improving that is outside of the scope of this PR :)
}); | ||
|
||
function typeResolver(source, context, info, abstractType) { | ||
return schema.getPossibleTypes(abstractType)[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this line to something like return schema.getPossibleTypes(abstractType).find(graphQLType => graphQLType.name === "FooObject");
. Makes it clearer what we are returning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Neitsch Great suggestion 👍
One thing is that it's enough to return string with type name from typeResolve
so matching type by name looks unnecessary if you can just return name.
So I just change it to be more explicit.
9f605b4
to
3b60a35
Compare
@Neitsch Yes, exactly 👍 |
Having global default
typeResolver
is very useful for writing GraphQL proxies and for mocking missing types. For example it would be very easy to mock types with missing resolver like this: