getTypeMap() does not include input types used in directives #1189
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? |
src/type/schema.js
Outdated
| @@ -269,3 +278,10 @@ function typeMapReducer(map: TypeMap, type: ?GraphQLType): TypeMap { | |||
|
|
|||
| return reducedMap; | |||
| } | |||
|
|
|||
| 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.
src/type/schema.js
Outdated
| 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
src/type/schema.js
Outdated
| @@ -13,6 +13,7 @@ import { | |||
| 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. |
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.