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] Proposed change to directive location introspection #152

Merged
merged 1 commit into from
Mar 22, 2016

Conversation

leebyron
Copy link
Collaborator

This specifies graphql/graphql-js#317

This proposes a change to how we represent the ability to validate the locations of directives via introspection.

Specifically, this removes onField, onFragment, and onOperation in favor of locations which is a list of __DirectiveLocation.

In addition, this changes the specification of the @skip and @include directives to no longer allow their use on fragment directives (but are still allowed on fragment spreads and inline fragments). Because of this, a section on "merging fragment directives" is no longer useful.

Rationale:

This allows for a more fine-grained validation of directive placement, now you can assert that a directive is allowed on queries but not mutations, or allowed on fragment definitions but not on fragment spreads.

Also, this makes expanding the locations a directive is allowed to be placed easier to do, as future expansions will not affect the introspection API. This should be considered a prereq to supporting directives in more locations.

Finally, this is a prereq to a forthcoming RFC to add directives to the type schema language, one of the last missing pieces to represent a full schema using this language.

Drawbacks:

Any change to the introspection API is a challenge. Tools like Graph_i_QL should continue to support the older representation if possible.

This proposes a change to how we represent the ability to validate the locations of directives via introspection.

Specifically, this *removes* `onField`, `onFragment`, and `onOperation` in favor of `locations` which is a list of `__DirectiveLocation`.

In addition, this changes the specification of the `@skip` and `@include` directives to no longer allow their use on fragment directives (but are still allowed on fragment spreads and inline fragments). Because of this, a section on "merging fragment directives" is no longer useful.

**Rationale:**

This allows for a more fine-grained validation of directive placement, now you can assert that a directive is allowed on queries but not mutations, or allowed on fragment definitions but not on fragment spreads.

Also, this makes expanding the locations a directive is allowed to be placed easier to do, as future expansions will not affect the introspection API. This should be considered a prereq to supporting directives in more locations.

Finally, this is a prereq to a forthcoming RFC to add directives to the type schema language, one of the last missing pieces to represent a full schema using this language.

**Drawbacks:**

Any change to the introspection API is a challenge. Tools like Graph*i*QL should continue to support the older representation if possible.
leebyron added a commit that referenced this pull request Mar 22, 2016
[RFC] Proposed change to directive location introspection
@leebyron leebyron merged commit ea790f2 into master Mar 22, 2016
@leebyron leebyron deleted the fine-grain-directives branch March 22, 2016 20:04
kliuless added a commit to tribune/graphql-ruby that referenced this pull request Apr 21, 2016
See graphql/graphql-spec#152
This is temporary until GraphiQL is updated.
kliuless added a commit to tribune/graphql-ruby that referenced this pull request Apr 25, 2016
See graphql/graphql-spec#152
This is temporary until GraphiQL is updated.
andimarek added a commit to graphql-java/graphql-java that referenced this pull request May 19, 2020
@andimarek
Copy link
Contributor

@leebyron we just removed these fields from graphql java: 3,5 years after this PR ;-)

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