-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
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:
Rather than:
Seems reasonable, to require the directive to decide if it wants to accept multiple inputs. |
Exactly. I think the latter API is more explicit to the possible behavior. |
fieldA @skip(if: $foo) | ||
fieldB @skip(if: $bar) | ||
} | ||
``` |
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.
A particularly interesting example is that this is also valid:
fieldA @skip(if: $foo)
fieldA @skip(if: $bar)
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.
Great point - I should add that
ae87499
to
65d1512
Compare
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
65d1512
to
8977618
Compare
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. |
This implements graphql/graphql-spec#229, in order to adhere with the October2016 edition of the spec.
This implements graphql/graphql-spec#229, in order to adhere with the October2016 edition of the spec.
…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
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