Merged
Conversation
Contributor
Author
|
I'll start discussion here in the impl, and once everything is covered, I'll ensure the spec matches this behavior. |
Contributor
|
Yeah, I think this is a big improvement; I like the |
Contributor
Author
|
I'll wait to land to give @schrockn and perhaps others a chance to look, and will get started on a PR for respective spec changes |
Contributor
|
This is an excellent change in terms of spec. Love it |
I think we have gotten directives slightly wrong. Since we're still in working draft, I think we should fix this:
Old:
```
{
foo @if: true
bar @unless: true
}
```
New:
```
{
foo @include(if: true)
bar @Skip(if: true)
}
```
Now directives are just as powerful as fields in their ability to accept multiple argument values. More importantly, this is a *simplification* as directive arguments and field arguments are syntactically *identical*, and semantically extremely similar, allowing us to reuse spec, grammar, and impl for both.
This simplification is really valuable to fix some issues with directives today. Directive value nullability is poorly (and probably incorrectly) defined in the current form. What does a non-null type on a directive today mean? Directives cannot be required. Now the rules are the same as field arguments.
This also helps us support future directives that have no arguments, or directives that have multiple arguments. `@defer` alone is perfectly fine, and `@defer(priority: 3)` is much easier to read than `@defer: 3`.
So if we do this, then `@if` and `@unless` are replaced by `@include` and `@skip` respectively. I think this is helpful as it describes more accurately what the directives' jobs are.
So grammar becomes:
```
Directive : Name Arguments?
```
And the directive value validation rule is merged into the argument validation rule.
Contributor
Author
|
Cool, everyone in favor - in it goes. Spec text next. |
leebyron
added a commit
to graphql/graphql-spec
that referenced
this pull request
Jul 6, 2015
Here is spec language mirroring the changes in graphql/graphql-js#31. Changed: * Directive grammar changed to `@ Name Arguments?` * "Field Arguments" changed to "Arguments" almost everywhere to be more generic. * Argument validation rules made more generic to handle directives cases. * Directive validation simplified. * Directive explaination in Language now matches current state. * @include and @Skip documented in Type System
leebyron
added a commit
to graphql/graphql-spec
that referenced
this pull request
Jul 7, 2015
Here is spec language mirroring the changes in graphql/graphql-js#31. Changed: * Directive grammar changed to `@ Name Arguments?` * "Field Arguments" changed to "Arguments" almost everywhere to be more generic. * Argument validation rules made more generic to handle directives cases. * Directive validation simplified. * Directive explaination in Language now matches current state. * @include and @Skip documented in Type System
leebyron
added a commit
to graphql/graphql-spec
that referenced
this pull request
Jul 7, 2015
Here is spec language mirroring the changes in graphql/graphql-js#31. Changed: * Directive grammar changed to `@ Name Arguments?` * "Field Arguments" changed to "Arguments" almost everywhere to be more generic. * Argument validation rules made more generic to handle directives cases. * Directive validation simplified. * Directive explaination in Language now matches current state. * @include and @Skip documented in Type System
dittos
added a commit
to dittos/graphqllib
that referenced
this pull request
Jul 7, 2015
joemcbride
added a commit
to graphql-dotnet/graphql-dotnet
that referenced
this pull request
Jul 10, 2015
* Update to new directives format graphql/graphql-js#31 * Enable query arguments: human(id: "123") * Update boolean coerce values * Build fragment spread in AST
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I think we have gotten directives slightly wrong. Since we're still in working draft, I think we should fix this:
Old:
New:
Now directives are just as powerful as fields in their ability to accept multiple argument values. More importantly, this is a simplification as directive arguments and field arguments are syntactically identical, and semantically extremely similar, allowing us to reuse spec, grammar, and impl for both.
This simplification is really valuable to fix some issues with directives today. Directive value nullability is poorly (and probably incorrectly) defined in the current form. What does a non-null type on a directive today mean? Directives cannot be required. Now the rules are the same as field arguments.
This also helps us support future directives that have no arguments or optional arguments, or directives that have multiple arguments.
@deferalone is perfectly fine, and@defer(priority: 3)is much easier to read than@defer: 3.So if we do this, then
@ifand@unlessare replaced by@includeand@skiprespectively. I think this is helpful as it describes more accurately what the directives' jobs are.So grammar becomes:
And the directive value validation rule is merged into the argument validation rule.