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

[RFC] Add Validation rule for unique directives per location. #229

Merged
merged 1 commit into from Oct 29, 2016

Conversation

leebyron
Copy link
Collaborator

This rule was suggested by @jjergus (#223) to remove ambiguity from execution where the @skip and @ignore directive rules make the assumption that only one is defined per selection.

In general I think this assumption should be upheld by valid queries overall, not just for @skip and @include so that it can be relied upon for any usage of directives. For example, this allows a potential optimization to use a hashmap keyed on directive name to represent the directives at a location rather than an array.

Closes #223

@stubailo
Copy link
Contributor

I guess this means that certain directives will need to include array arguments, rather than having one argument and using the directive multiple times, eg:

{
  comments @refetchOn(channels: ["newComment", "removedComment"])
}

Rather than:

{
  comments @refetchOn(channel: "newComment") @refetchOn(channel: "removedComment")
}

Seems reasonable, to require the directive to decide if it wants to accept multiple inputs.

@leebyron
Copy link
Collaborator Author

Exactly. I think the latter API is more explicit to the possible behavior.

fieldA @skip(if: $foo)
fieldB @skip(if: $bar)
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A particularly interesting example is that this is also valid:

fieldA @skip(if: $foo)
fieldA @skip(if: $bar)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point - I should add that

This rule was suggested by @jjergus (#223) to remove ambiguity from execution where the `@skip` and `@ignore` directive rules make the assumption that only one is defined per selection.

In general I think this assumption should be upheld by valid queries overall, not just for `@skip` and `@include` so that it can be relied upon for any usage of directives. For example, this allows a potential optimization to use a hashmap keyed on directive name to represent the directives at a location rather than an array.

Closes #223
@leebyron
Copy link
Collaborator Author

Further rationale after an offline discussion with @dschafer:

While this is explicitly a limiting change and therefore potentially breaking, by reducing what is possible now it increases future option value. This change is one that we could always change back in some capacity in the future in a non-breaking way should an explicit need arise. Barring any such need today, it's in our best interest to preserve that option-value.

@leebyron leebyron merged commit 3fc8a94 into master Oct 29, 2016
@leebyron leebyron deleted the directive-uniqueness branch October 29, 2016 00:41
leebyron added a commit to graphql/graphql-js that referenced this pull request Nov 1, 2016
This implements graphql/graphql-spec#229, in order to adhere with the October2016 edition of the spec.
leebyron added a commit to graphql/graphql-js that referenced this pull request Nov 2, 2016
This implements graphql/graphql-spec#229, in order to adhere with the October2016 edition of the spec.
IvanGoncharov pushed a commit to IvanGoncharov/graphql that referenced this pull request Jun 17, 2017
…l#229)

This rule was suggested by @jjergus (graphql#223) to remove ambiguity from execution where the `@skip` and `@ignore` directive rules make the assumption that only one is defined per selection.

In general I think this assumption should be upheld by valid queries overall, not just for `@skip` and `@include` so that it can be relied upon for any usage of directives. For example, this allows a potential optimization to use a hashmap keyed on directive name to represent the directives at a location rather than an array.

Closes graphql#223
@OlegIlyenko OlegIlyenko mentioned this pull request Apr 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants