Skip to content
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

Merged
merged 1 commit into from
Jul 6, 2015
Merged

Reworking of Directives #31

merged 1 commit into from
Jul 6, 2015

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented Jul 6, 2015

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 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:

Directive : Name Arguments?

And the directive value validation rule is merged into the argument validation rule.

@leebyron
Copy link
Contributor Author

leebyron commented Jul 6, 2015

I'll start discussion here in the impl, and once everything is covered, I'll ensure the spec matches this behavior.

@dschafer
Copy link
Contributor

dschafer commented Jul 6, 2015

Yeah, I think this is a big improvement; I like the skip and include names too, I think that's clearer than if and unless.

@leebyron
Copy link
Contributor Author

leebyron commented Jul 6, 2015

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

@schrockn-zz
Copy link
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.
@leebyron
Copy link
Contributor Author

leebyron commented Jul 6, 2015

Cool, everyone in favor - in it goes. Spec text next.

leebyron added a commit that referenced this pull request Jul 6, 2015
@leebyron leebyron merged commit 6ec76f4 into master Jul 6, 2015
@leebyron leebyron deleted the directives branch July 6, 2015 22:41
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants