-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Reworking of Directives #31
Conversation
I'll start discussion here in the impl, and once everything is covered, I'll ensure the spec matches this behavior. |
Yeah, I think this is a big improvement; I like the |
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 |
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.
Cool, everyone in favor - in it goes. Spec text next. |
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
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
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
* Update to new directives format graphql/graphql-js#31 * Enable query arguments: human(id: "123") * Update boolean coerce values * Build fragment spread in AST
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.
@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:
And the directive value validation rule is merged into the argument validation rule.