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] Limit uniqueness to @skip, @include and @deprecated directives #471

Closed
wants to merge 2 commits into from

Conversation

OlegIlyenko
Copy link
Contributor

@OlegIlyenko OlegIlyenko commented Jun 24, 2018

This proposal directly relates to a discussion in #429. As was discussed at the latest WG meeting, I'm creating several alternative proposals. This one implements proposed solution 3. Limit the validation to only @skip and @include directives.

It limits the scope of "Directives Are Unique Per Location" to @skip and @include directives only.

This proposal is mutually exclusive with other alternative proposals:

@leebyron @IvanGoncharov @jjergus I would appreciate your reviews.

Closes #429

@IvanGoncharov
Copy link
Member

@OlegIlyenko I will try to do detail review in the next couple of days. But from top of my head, it also should include @deprecate.

@OlegIlyenko OlegIlyenko changed the title [RFC] Limit uniqueness to @skip and @include directives [RFC] Limit uniqueness to @skip, @include and @deprecated directives Jun 25, 2018
@OlegIlyenko
Copy link
Contributor Author

@IvanGoncharov thanks! Good point, I focused on @skip and @include and completely forgot about @deprecated. I updated the PR.

@IvanGoncharov
Copy link
Member

Partly alternative proposal based on #429 (comment): I think we can define the behavior of multiple @skip/@include.
We could change this:
http://facebook.github.io/graphql/June2018/#sec--include

Neither @Skip nor @include has precedence over the other. In the case that both the @Skip and @include directives are provided in on the same the field or fragment, it must be queried only if the @Skip condition is false and the @include condition is true

into this:

Neither @Skip nor @include has precedence over the other or other instance of the same directive. In the case that multiple @Skip or @include directives are provided in on the same the field or fragment, it must be queried only if all @Skip conditions are false and all @include conditions are true

This also require to change this algorithm:
http://facebook.github.io/graphql/June2018/#CollectFields()

Benefits: Currently you can model only OR for includes:

{
   field @include(if: $foo)
   field @include(if: $bar)
}

These proposal allows to do the opossite operation:

{
   field @include(if: $foo) @include(if: $bar)
}

@OlegIlyenko
Copy link
Contributor Author

OlegIlyenko commented Jun 26, 2018

I like this suggestion. This lifts the constraint and defines intuitive disambiguation rules.

I still struggle with the question of why such a strong line is drawn between:

  1. field @skip(if: false) @skip(if: true) (disallowed by validation)
  2. field @skip(if: false) @include(if: false) (allowed)

both variations are semantically identical (and raise the same ambiguity issues). In my opinion, if we keep that validation around, then it probably should be reason other than ambiguity between @skip and @include. Though it makes sense for @deprecated since there is no straightforward way to disambiguate 2 of these (except just to pick first or last, which is probably suboptimal).

Maybe I should prepare the third PR that is a combination of #472 + non-unique @skip and include + the change in the text that you have suggested. What does everybody think about it?

@leebyron leebyron added 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) and removed RFC labels Oct 2, 2018
@leebyron
Copy link
Collaborator

leebyron commented Oct 2, 2018

To follow up from the WG meeting: I think we should extract the skip/include rules from the deprecated to ensure we're leading to the right solution for each. I'm excited about the direction of clearing up ambiguity when using skip and include together

@OlegIlyenko
Copy link
Contributor Author

OlegIlyenko commented Oct 3, 2018

@leebyron sure thing. i think I would prefer to first finish #470 and #472 to not mix things too much. Later on I think this PR needs to be closed and I can open another one which is dedicated to skip/include interaction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limiting the scope of "Directives Are Unique Per Location" validation
4 participants