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] Allow directives on variable definitions #510

Merged
merged 1 commit into from Oct 1, 2018
Merged

Conversation

@mjmahone
Copy link
Contributor

mjmahone commented Aug 29, 2018

Redo of #486. Will wait for discussion at the next WG meeting.

This is currently implemented under a feature flag in graphql-js: https://github.com/graphql/graphql-js/blob/master/src/language/parser.js#L128

Now that directives are gaining more widespread adoption, there have been multiple use cases I've seen where people want directives on variable definitions, but have to resort instead to adding them on the query definition with arguments.

An example of this: some query variable may only make sense for the client. As an example, if you have a local cache and you need a variable to differentiate different runs of the same query against that cahce. Or if you have a query being run with a different set of fragments, but the client code initiating that query needs to conform to the same API. The way to describe this might be:

query SomeQuery(
  $client_var: Boolean = false @client_only
  $server_var: Boolean = true
) { ... }

The client could strip $client_var before persisting it to the server as

query SomeQuery(
  $server_var: Boolean = true
) { ... }

With our current set of directive locations, this would have to be implemented on the query definition like:

query SomeQuery(
  $client_var: Boolean = false
  $server_var: Boolean = true
) @client_only(variables: ['client_var']) { ... }

This version has a lot more validation that needs to happen (for instance, that the string argument provided is actually a variable defined on the query), and is more disconnected from the intention: to strip the client-only variable, you now have to visit all of the query's variables, rather than just stripping the node that explicitly has the directive on it.

Redo of #486. Will wait discussion at the next WG meeting.
@mjmahone mjmahone added the RFC label Aug 29, 2018
mjmahone added a commit to mjmahone/graphql-wg that referenced this pull request Aug 29, 2018
See graphql/graphql-spec#510 for background: basically, is the API correct. It matches the API for directives on field argument definitions, and is (I believe) the most glaring "hole" in where we allow you to add directives.
leebyron added a commit to graphql/graphql-wg that referenced this pull request Aug 31, 2018
* Add myself to Attendees

* Add discussion of variable definition directives

See graphql/graphql-spec#510 for background: basically, is the API correct. It matches the API for directives on field argument definitions, and is (I believe) the most glaring "hole" in where we allow you to add directives.
@leebyron leebyron merged commit 760753e into master Oct 1, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@leebyron

This comment has been minimized.

Copy link
Collaborator

leebyron commented Oct 1, 2018

🙌

@leebyron leebyron deleted the var-def-directives branch Oct 1, 2018
@leebyron leebyron added 🏁 Accepted (RFC 3) and removed RFC labels Oct 2, 2018
@lirsacc lirsacc mentioned this pull request Jan 28, 2020
4 of 9 tasks complete
lirsacc added a commit to lirsacc/py-gql that referenced this pull request Jan 28, 2020
This adds language support for directives on variable definitions
following graphql/graphql-spec#510.

This is **only** language support so servers don't choke when
receiving queries using this feature.
@michaelstaib michaelstaib mentioned this pull request Jan 29, 2020
3 of 11 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.