-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improve clarity of built-in directives #633
Conversation
spec/Section 3 -- Type System.md
Outdated
the set of directives from the `__Schema` introspection type. | ||
|
||
When representing a GraphQL schema using the type system definition language, | ||
both `@skip`, `@include` and `@deprecated` directives can be omitted for brevity. |
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.
Since now we have definition for built-in directives
it's better to use it:
both `@skip`, `@include` and `@deprecated` directives can be omitted for brevity. | |
built-in directives can be omitted for brevity. |
Also, I checked #597 and says:
all built-in scalars must be omitted for brevity.
So I think you were right initially and we should use must
for consistency.
Sorry for the confusion 🤦♂
both `@skip`, `@include` and `@deprecated` directives can be omitted for brevity. | |
built-in directives must be omitted for brevity. |
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
is used in the scalars section, so here I used it too
Anything else to fix? |
bump |
spec/Section 3 -- Type System.md
Outdated
GraphQL implementations should provide the `@skip` and `@include` directives. | ||
|
||
GraphQL implementations that support the type system definition language must | ||
provide the `@deprecated` directive if representing deprecated portions of | ||
the schema. | ||
|
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.
I don't think this section needs to be removed
spec/Section 3 -- Type System.md
Outdated
|
||
GraphQL implementations should provide the `@skip` and `@include` directives. | ||
When returning the set of directives from the `__Schema` introspection type, both | ||
`@skip` and `@include` directives must be included. |
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.
Since @skip
and @include
directives only should be provided, a service can omit them, if for some reason its executor cannot handle them.
They should be present in introspection if the service supports them.
spec/Section 3 -- Type System.md
Outdated
@@ -1692,6 +1686,20 @@ While defining a directive, it must not reference itself directly or indirectly: | |||
directive @invalidExample(arg: String @invalidExample) on ARGUMENT_DEFINITION | |||
``` | |||
|
|||
**Built-in Directives** |
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.
It seems like the confusion is specifically about introspection, so additional clarity should be added to that section rather than including a new sub-section here. Currently this section seems to mostly just repeat the content from the existing @skip
, @include
and @deprecated
sections. As we add additional directives over time we should avoid having too many places we need to edit to expand that list.
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.
I'm also concerned about "built-ins" becoming an ambiguous term here since the directives defined are not all required to be provided by a service.
I'd prefer either "The directives specified by this document" as representing the full set of non-custom directives, or something like "directives specified by this document and provided by a GraphQL service" if referring to the subset actually supported
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.
My implementation of schema introspection add includeBuiltin
param for types
and directives
fields. It's up to the user whether he want to query builtin types/directives or not.
type __Schema {
description: String
types(includeBuiltin: Boolean = false): [__Type!]!
queryType: __Type!
mutationType: __Type
subscriptionType: __Type
directives(includeBuiltin: Boolean = false): [__Directive!]!
}
my implementation is able to query the introspection schema itself, that's why I added those params. to avoid confusion for new users.
I've added some changes that addressed my earlier review. @sungam3r I'd love your final look at this |
Thanks @leebyron . Only 3 minor comments. |
Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
e5d241d
to
6c81ed8
Compare
fixes #632
relates to #597