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 #486

Merged
merged 2 commits into from Aug 7, 2018
Merged

[RFC] Allow directives on variable definitions #486

merged 2 commits into from Aug 7, 2018

Conversation

mjmahone
Copy link
Contributor

@mjmahone mjmahone commented Jul 28, 2018

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.

@@ -162,7 +162,7 @@ ObjectField[Const] : Name : Value[?Const]

VariableDefinitions : ( VariableDefinition+ )

VariableDefinition : Variable : Type DefaultValue?
VariableDefinition : Variable : Type Directives? DefaultValue?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

mjmahone commented Aug 7, 2018

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
Copy link
Contributor Author

mjmahone commented Aug 7, 2018

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
Copy link
Collaborator

leebyron commented Aug 7, 2018

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
Copy link
Collaborator

leebyron commented Aug 7, 2018

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
Copy link
Contributor Author

mjmahone commented Aug 7, 2018

@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.

mjmahone added a commit that referenced this pull request Aug 29, 2018
Redo of #486. Will wait discussion at the next WG meeting.
leebyron pushed a commit that referenced this pull request Oct 1, 2018
Redo of #486. Will wait discussion at the next WG meeting.
@leebyron leebyron removed the RFC label Oct 2, 2018
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

5 participants