-
-
Notifications
You must be signed in to change notification settings - Fork 247
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
Render directives at the top of the schema #1270
Conversation
case __DirectiveLocation.QUERY => "QUERY" | ||
case __DirectiveLocation.MUTATION => "MUTATION" | ||
case __DirectiveLocation.SUBSCRIPTION => "SUBSCRIPTION" | ||
case __DirectiveLocation.FIELD => "FIELD" | ||
case __DirectiveLocation.FRAGMENT_DEFINITION => "FRAGMENT_DEFINITION" | ||
case __DirectiveLocation.FRAGMENT_SPREAD => "FRAGMENT_SPREAD" | ||
case __DirectiveLocation.INLINE_FRAGMENT => "INLINE_FRAGMENT" | ||
case __DirectiveLocation.SCHEMA => "SCHEMA" | ||
case __DirectiveLocation.SCALAR => "SCALAR" | ||
case __DirectiveLocation.OBJECT => "OBJECT" | ||
case __DirectiveLocation.FIELD_DEFINITION => "FIELD_DEFINITION" | ||
case __DirectiveLocation.ARGUMENT_DEFINITION => "ARGUMENT_DEFINITION" | ||
case __DirectiveLocation.INTERFACE => "INTERFACE" | ||
case __DirectiveLocation.UNION => "UNION" | ||
case __DirectiveLocation.ENUM => "ENUM" | ||
case __DirectiveLocation.ENUM_VALUE => "ENUM_VALUE" | ||
case __DirectiveLocation.INPUT_OBJECT => "INPUT_OBJECT" | ||
case __DirectiveLocation.INPUT_FIELD_DEFINITION => "INPUT_FIELD_DEFINITION" |
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.
This is a copy paste to the __DirectiveLocation
code. Wasn't sure if we wanted to somehow encode the string version of the location in the case object
?
} | ||
val directiveLocations = locationStrings.mkString(" | ") | ||
|
||
val body = s"""directive @${directive.name}${inputs} repeatable on ${directiveLocations}""".stripMargin |
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.
Should we add repeatable: Boolean
to directives with default to true?
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.
Oh shoot I meant to ask about this. Right now it's hard-coded as repeatable
. I didn't look at what we did for this with the introspection API but it would be good to allow it to be set as non-repeatable.
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.
Ah this keyword is new in the 2021 spec, which is why it was not there before. Would be nice to add (otherwise, don't include the keyword).
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.
Cool, let me see what I can do about adding it.
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.
On the top of my head, it should be added in the parser, in the Directive
and __Directive
classes, and maybe a validation rule should be added to check there is only one given if not repeatable.
If that's too much, it can be done in another PR 😄
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.
Cool. Yeah I will remove the repeatable
statement for now, and then do another PR to add repeatable
.
@ghostdogpr removed the I'll start on the second PR to add |
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.
Thanks!
Any custom
__Directives
added to the schema as part of theGraphQL.graphQL
method do not render when thegraph.render()
method is called. This PR adds support for that.