-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add support for directives on directive definitions #1206
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
Changes from all commits
7167aa9
40fc00a
5595ab3
0ef5222
d2c5424
610d8db
a257894
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,7 @@ TypeSystemExtension : | |
|
|
||
| - SchemaExtension | ||
| - TypeExtension | ||
| - DirectiveExtension | ||
|
|
||
| Type system extensions are used to represent a GraphQL type system which has | ||
| been extended from some previous type system. For example, this might be used by | ||
|
|
@@ -2033,7 +2034,7 @@ Following are examples of result coercion with various types and values: | |
| ## Directives | ||
|
|
||
| DirectiveDefinition : Description? directive @ Name ArgumentsDefinition? | ||
| `repeatable`? on DirectiveLocations | ||
| Directives[Const]? `repeatable`? on DirectiveLocations | ||
|
|
||
| DirectiveLocations : | ||
|
|
||
|
|
@@ -2069,6 +2070,7 @@ TypeSystemDirectiveLocation : one of | |
| - `ENUM_VALUE` | ||
| - `INPUT_OBJECT` | ||
| - `INPUT_FIELD_DEFINITION` | ||
| - `DIRECTIVE_DEFINITION` | ||
|
|
||
| A GraphQL schema describes directives which are used to annotate various parts | ||
| of a GraphQL document as an indicator that they should be evaluated differently | ||
|
|
@@ -2242,13 +2244,13 @@ condition is false. | |
| ```graphql | ||
| directive @deprecated( | ||
| reason: String! = "No longer supported" | ||
| ) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE | ||
| ) on FIELD_DEFINITION | ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION | ENUM_VALUE | DIRECTIVE_DEFINITION | ||
| ``` | ||
|
|
||
| The `@deprecated` _built-in directive_ is used within the type system definition | ||
| language to indicate deprecated portions of a GraphQL service's schema, such as | ||
| deprecated fields on a type, arguments on a field, input fields on an input | ||
| type, or values of an enum type. | ||
| type, values of an enum type, or directives. | ||
|
|
||
| Deprecations include a reason for why it is deprecated, which is formatted using | ||
| Markdown syntax (as specified by [CommonMark](https://commonmark.org/)). | ||
|
|
@@ -2321,3 +2323,24 @@ input UserUniqueCondition @oneOf { | |
| organizationAndEmail: OrganizationAndEmailInput | ||
| } | ||
| ``` | ||
|
|
||
| ### Directive Extensions | ||
|
|
||
| DirectiveExtension : extend directive @ Name Directives[Const] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is interesting: should we be able to extend a directive with new arguments? We can extend a type with new fields that themselves have new arguments. There is even a special Schema Coordinate for directive arguments.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! In my opinion, I think yes it would probably make sense in principle to have that ability, even if for consistency alone. Curious to know what others think!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it would be controversial, but it does make sense to stack into two PRs to make discussion specifically about argument extension more focused.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mjmahone Am I correct that https://gaps.graphql.org/GAP-33/draft/ would also solve this use case?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, GAP-33 assumes that this is already available. But I think I should iterate on that GAP to make inner extensions use a different syntax because as-is it's not good! |
||
|
|
||
| Directive extensions are used to represent a directive which has been extended | ||
| from some previous directive. For example, this might be used by a GraphQL tool | ||
| or service which adds directives to an existing directive. | ||
|
|
||
| **Type Validation** | ||
|
|
||
| Directive extensions have the potential to be invalid if incorrectly defined. | ||
|
|
||
| 1. The previous directive must already be defined. | ||
| 2. Any non-repeatable directives provided must not already apply to the previous | ||
| directive. | ||
| 3. Any directives provided must not contain the use of a Directive which | ||
|
yaacovCR marked this conversation as resolved.
|
||
| references the previous directive directly. | ||
| 4. Any directives provided must not contain the use of a Directive which | ||
| references the previous directive indirectly by referencing a Type or | ||
| Directive which transitively includes a reference to the previous Directive. | ||
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 hesitant to add this, since I think the whole
includeDeprecateddance was a mistake. I think I'd rather see theincludeDeprecatedargument be itself deprecated across the board, but it defaulting to= falsemakes this challenging.Uh oh!
There was an error while loading. Please reload this page.
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.
Yeah I certainly agree about
includeDeprecated's benefits being outweighed by its drawbacks. Having it here was about consistency - but maybe if we're planning on deprecating it, we can start by not having it here! Let's discuss this at the next WG.