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

Directive precedence should be explicit, not implicit #65

Closed
savaki opened this issue Jul 21, 2015 · 2 comments
Closed

Directive precedence should be explicit, not implicit #65

savaki opened this issue Jul 21, 2015 · 2 comments

Comments

@savaki
Copy link

savaki commented Jul 21, 2015

The draft states that if both the @Skip and @include directives are present, the @Skip directive should take precede. This strikes me as confusing as it opens the door to others creating directives with implicit precedence.

Wouldn't it be simpler and less confusing to say precedence is left to write in the order it was written?

If fragments are involved, then we could linearize the dependencies by saying depth first. Then there are no ambiguities.

@leebyron
Copy link
Collaborator

Thanks for the recommendation.

The goal is to be as simple as possible, so I appreciate the argument. Simplicity also led to the current behavior, though primarily simplicity of implementation. Adding the additional step of order-dependence makes this more complicated.

There is also a desire for the directives to be un-ordered so they can be safely represented as a Set rather than a List internally, this can be a simplifying force for implementations and removes a potential area for mistakes by accidental re-ordering.

I have another suggestion that I'm interested in getting your feedback on, hopefully it solves your concerns:

As currently specced:

  1. If selection provides the directive @skip, let skipDirective be that directive.
    1. If skipDirective‘s if argument is true, continue with the next selection in selectionSet.
  2. If selection provides the directive @include, let includeDirective be that directive.
    1. If includeDirective‘s if argument is false, continue with the next selection in selectionSet.

We get this truth table if both skip and include are present:

@skip(if: true) @skip(if: false) Not provided
@include(if: false) skipped skipped skipped
@include(if: true) skipped included included
Not provided skipped included included

Based on this, I think we can reframe the relationship as written in the spec to be a formal AND. That would have an identical truth table, but would not contain any implicitly ordered relationship between the two nor would it rely on any explicit ordering (due to the commutative property).

Another nice outcome of this is that the spec could potentially be written to allow multiple @skip or @include directives, all AND'd together, so any single @skip(if: true) or @include(if: false) in the list would result in a skip. I'm not sure if this is a good idea or not yet (it could be a bag of worms to allow for multiple directives) but I thought it was interesting to bring up.

@leebyron
Copy link
Collaborator

leebyron commented Apr 7, 2016

Closing since this note has been added to the spec about there being no precedence in this case: http://facebook.github.io/graphql/#sec--include

@leebyron leebyron closed this as completed Apr 7, 2016
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

No branches or pull requests

2 participants