[RFC] Allow directives on variable definitions

[mjmahone]

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.

[mjmahone]

Putting the directives before the default value seems to conform closer to how directives are used in general, but I could be convinced that this should be VariableDefinition : Variable : Type DefaultValue? Directives? instead.

The reason this seems to match better, is due to things like Union and Fragment definitions.

For a union definition, it looks like:
union SomeUnion @some_directive = SomeObject | SomeOtherObject

For a fragment, it's:
fragment SomeFragment on SomeObject @directives { ... }

So the directive always goes after the type is defined, but before the definition's "value" is defined. In this case, a default value is analogous to a fragment's FieldSet, or a union's UnionMemberTypes, I think.

[IvanGoncharov]

@mjmahone It should be after default value to match existing syntax for input value definition:
http://facebook.github.io/graphql/June2018/#InputValueDefinition

[IvanGoncharov]

Also please use Directives[Const] since you can't use variables before you defined them.

[mjmahone]

Wow I missed the InputValueDefinition. That seems like an unfortunate choice we made, as it doesn't feel like the natural "fit" given where directives usually are put (and how rare directives on InputValueDefinitions are likely to be). But given that, I agree, much better to make the change, and keep the two aligned.

And the const-ness is a very good point: you wouldn't want variables inside your directive arguments as it's impossible to define them beforehand. Thank you!

[mjmahone]

I'm merging this, as this seems more like an oversight from when we last added new directive locations, rather than a fundamental language change.

[mjmahone]

Pinging @leebyron in case there were any objections: I've had this up for a while, and there didn't seem to be any controversy. If there are any concerns with this, please let me know, and I can roll it back if needed.

[leebyron]

I just reverted this. We always discuss spec proposals at WG meetings before merging and please allow me to merge any spec significant changes.

Also, spec changes typically come after successful experimentation in code

[leebyron]

I think this is a great change, but let’s get consensus at the next WG meeting before merging.

Also, to avoid confusion (I was confused) you should edit your examples to be valid grammar. My feedback was going to be to place directives after default value, but I see you already made that change

[mjmahone]

@leebyron sorry, wasn't sure what RFC process was: we need this internally for the 14.0 release of graphql-js, so I was under the impression I needed to make the changes in graphql spec first. Got my dependencies reversed, sorry about that. Will put this back up for WG discussion.