getTypeMap() does not include input types used in directives#1189
getTypeMap() does not include input types used in directives#1189leebyron merged 3 commits intographql:masterfrom
Conversation
Yogu
force-pushed
the
directive-type-map
branch
3 times, most recently
from
58d66d8 to
ed0f453
Compare
Yogu
force-pushed
the
directive-type-map
branch
from
ed0f453 to
2616ced
Compare
@Yogu You absolutely right 👍 Do you want to fix it yourself? |
|
Ok great, I'll push a fix tomorrow. However, I would have first collected the types of directive arguments, and then used the |
Yogu
force-pushed
the
directive-type-map
branch
7 times, most recently
from
1a85ba2 to
7321a18
Compare
|
@IvanGoncharov Can you have a look on the fix? |
| } | ||
|
|
||
| function getDirectiveArgTypes(directive: GraphQLDirective) { | ||
| if (!isDirective(directive)) { |
There was a problem hiding this comment.
Why do you need this check?
There was a problem hiding this comment.
There is a test that validateSchema throws a specific error when directives is invalid (a string instead of a proper instance). new GraphQLSchema() does not perform this validation, so for the mentioned test to pass, the constructor needs to accept the invalid directive.
| if (!isDirective(directive)) { | ||
| return []; | ||
| } | ||
| return directive.args.map(arg => getNamedType(arg.type)); |
There was a problem hiding this comment.
@Yogu You don't need getNamedType here since it's already handled here:
https://github.com/Yogu/graphql-js/blob/7321a186854af3a095fe7ee52a2a1622dbf288b8/src/type/schema.js#L235-L237
| locations: ['OBJECT'], | ||
| args: { | ||
| arg: { | ||
| type: DirectiveInputType, |
There was a problem hiding this comment.
Would be great to also test wrapped type maybe as one more argument
Yogu
force-pushed
the
directive-type-map
branch
from
7321a18 to
afe4735
Compare
| isUnionType, | ||
| isInputObjectType, | ||
| isWrappingType, | ||
| getNamedType, |
There was a problem hiding this comment.
@Yogu Looks great now!
One last issue from me: Can you please remove this line?
It cause Travis build to fail:
|
This looks like it also won't work if a directive argument type is a list or non-null type. I would expect flow to also report an error with this PR |
@leebyron I initially had code to unwrap the named type, but @IvanGoncharov pointed out this should not be necessary. I'll add a test tomorrow to see if it works. |
|
Ah - I think you're right, it's just not clear from the code. |
|
@leebyron In Voyager we support selecting any type as a root of a graph: It would be great to reuse Also, it's very useful for schema stitching since you need to find a minimal set of dependencies for the every injected type. |
4 participants
The result of
getTypeMap()recursively includes all types referenced by root types, so I would expect it to include types referenced by directives, too.If this is not intended, I think a test asserting the opposite would be a good idea.