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: Schema Language Directives #376

Merged
merged 1 commit into from
May 6, 2016
Merged

RFC: Schema Language Directives #376

merged 1 commit into from
May 6, 2016

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented May 4, 2016

This implements adding directives to the experimental schema language by extending the locations a directive can be used.

Notice that this provides no semantic meaning to these directives - they are purely a mechanism for annotating an AST - however future directives which contain semantic meaning may be introduced in the future (the first will be @deprecated). More specifically this means that there is no change to the GraphQLSchema object and therefore also no changes to the introspection API.

Because this is purely an way to annotate definitions within an AST, it provides (only) the basis for implementing all the related ideas which require this syntactic change:

  • Field deprecation can be implemented by introducing a @deprecated directive and altering the existing buildASTSchema function to be aware of this.
  • Other metadata, similar to deprecation, can be built in the same way.
  • Instructions to code-generators.
  • Schema transforms (or "decorators") can be built as any function which accepts the AST, is aware of particular relevant directives, and produces a GraphQLSchema object - perhaps this concept can be built by extending buildASTSchema with a plugin model.

Also, because this implements schema directives by extending the available locations of the existing directive definitions, this means that directives must be well-defined before they can be used. This enables validation to ensure that schema directives are used only in the way they are intended to be used.

Many huge thanks to everyone who's contributed to the thought process behind how best to implement this kind of schema directive. First proposed by @igorcanadi in #265, then later refined by @clintwood in #334 and keenly described by @helfer in a recent apollo post.

There have been many subtle variations on the actual grammatic form for these annotations that we can boil down to these few decisions:

Should schema directives accept arguments in a similar form to operation directives?

Just about every proposal so far has recommended this, so I think this is emphatic agreement.

Should the "sigil" character match operation directives, or be something new?

Each proposal has suggested a new character (@directive, @@directive, +directive and %directive). I think it is prudent to have a unified syntax between language directives and schema directives as both a simplification and to ease any learning curve. By ensuring both kinds of directives appear the same, we can avoid any confusion between the two.

Should schema directives be placed before or after the relevant definition?

This has been contested and both proposed, with more proposals favoring before by citing python decorators and java annotations as prior art. There's a lot of validity to this argument but I believe this boils down to internal consistency within the language. Because the existing directives are placed after the relevant definition and this proposal simply extends the existing directives language, it makes sense to remain consistent and also place schema directives after (or within, as is consistent) their relevant definitions. The original directives syntax for GraphQL was inspired by https://capnproto.org/language.html#annotations, so this decision continues to follow that prior art.

@ghost ghost added the CLA Signed label May 4, 2016
This implements adding directives to the experimental schema language by extending the *locations* a directive can be used.

Notice that this provides no semantic meaning to these directives - they are purely a mechanism for annotating an AST - however future directives which contain semantic meaning may be introduced in the future (the first will be `@deprecated`).
@leebyron leebyron force-pushed the schema-directives branch from be4697d to fae564d Compare May 5, 2016 00:13
@leebyron
Copy link
Contributor Author

leebyron commented May 5, 2016

Also cc @OlegIlyenko, @freiksenet, and @josephsavona who have expressed opinions on this in the past.

Barring any strong objections, I'd like to move forward with this, and will then amend the (currently thin) graphql/graphql-spec#90

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.269% when pulling be4697d on schema-directives into 51362e7 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.269% when pulling fae564d on schema-directives into 51362e7 on master.

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented May 5, 2016

Thanks for putting this together, @leebyron! I don't have any objections.

I still feel that putting directives in front of the definitions would be a bit more comfortable. But I guess this comes from by background working with languages like java, scala, python, etc. I just feel that it's more common these days, thus it would be more familiar to most of the users. But it's just a minor thing.

Do you plan to represent descriptions in IDL with directives as well?

Also another question. There were a lot of different discussions around and directives/annotations, so the concept became a bit overloaded :) Here you explicitly mentioned that directives just annotate IDL AST nodes without any additional semantics. I was wondering whether you plan to define something that will expose additional (and user-defined) schema meta-information via introspection API (like, for instance, a list of annotations/directives on fields and types)? Several proposals mentioned this, if I remember correctly.

@leebyron
Copy link
Contributor Author

leebyron commented May 5, 2016

I think descriptions will be most ergonomically represented as comments above the described definition. That follows the javadoc prior art to some degree, but I think it will just feel more natural.

This proposal doesn't make suggestions as to how metadata in introspection or decorators (schema modifiers) should behave, but I'm excited about both ideas. What I'm hoping for here is just a common syntax we can use to start building both of those ideas.

@leebyron
Copy link
Contributor Author

leebyron commented May 5, 2016

And of course I don't mean to discredit that placing these directives before feels more natural coming from Python or Java. I agree with that! I just find internal consistency in the language more valuable.

@OlegIlyenko
Copy link
Contributor

OlegIlyenko commented May 5, 2016

👍 sounds great!

Also, it was not mentioned that much these discussions, I think this addition would be a great help with cross-language compatibility tests (graphql/graphql-spec#72). This makes it possible to define a set of directives which will control behavior of the resolve function for the tests.

case ENUM_VALUE_DEFINITION: return DirectiveLocation.ENUM_VALUE;
case INPUT_OBJECT_TYPE_DEFINITION: return DirectiveLocation.INPUT_OBJECT;
case INPUT_VALUE_DEFINITION:
const parentNode = ancestors[ancestors.length - 3];
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need to check that 'ancestors.length >= 3' or does parser guarantee that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parser all but guarantees it. The only way this could end up being negative is if you assemble an invalid AST to provide to the validator, and then you probably have other problems on your hands.

@igorcanadi
Copy link
Contributor

This is awesome. I really like how you separated the concept of schema directives from their semantic meaning in the schema and the idea of implementing schema transforms to give the directives ability to influence the schema metadata.

No objections from me, really looking forward to writing some directives on client-side types :)

@helfer
Copy link
Contributor

helfer commented May 5, 2016

@leebyron: This is great, really moves the conversation about schema metadata forward!

I'm guessing the next steps are to actually add the information to the schema generated by buildASTSchema and then decide how to expose it in schema introspection? I'd love to get started on that by opening a discussion thread, if it's okay with you?

@leebyron
Copy link
Contributor Author

leebyron commented May 5, 2016

Yeah! Those are open questions for sure. There are a lot of use cases for these schema directives discussed, and details to work out.

The immediate next steps after agreeing on and landing this syntax change will be introducing a @deprecated directive for the schema builder to use. Then I'm interested in furthering discussion for metadata and decorator uses

@helfer
Copy link
Contributor

helfer commented May 5, 2016

Oh, one more question: is there a reason the schema itself cannot have directives?

What I mean by the schema having a directive is something like this:

type Query {
  aString: String
}

schema @aDirective(foo: "bar"){
  query: Query
}

This would be similar to annotating the document in capnproto.

@leebyron
Copy link
Contributor Author

leebyron commented May 5, 2016

It's not that it cannot, I just didn't add it yet here. I think it's a reasonable thing to add, I'll follow up

@clintwood
Copy link

@leebyron, thanks for this, it looks great for it's intent!

If I've understood this RFC correctly, what it doesn't cover and which PR #334 proposes as a concept of annotations, is a formal way to:

  • add metadata to schema and query language and JavaScript Schema Definition via API
  • without needing to be 'well-define' (declared) upfront (as is done with directives)
  • and thus can be used (or ignored) by tooling at build-time and is available in the AST at runtime once parsed by the respective parsers (schema/query)

Apart from the above directives are very similar to the annotations concept.

I'm going to close PR #334 in favour of this one, although it doesn't fully cover the annotations concept but I believe I can work with this.

@robzhu
Copy link
Contributor

robzhu commented May 5, 2016

Your phased rollout plan is very thoughtful. You mention that we use the "@" sigil to be consistent with other languages, but we're sticking with the postfix notation for historical reasons. Do you intend to revisit the pre/postfix decision (for consistency with ES7 decorators)? If so, is this something that might affect the upcoming implementation of the first few core/example directives?

@leebyron
Copy link
Contributor Author

leebyron commented May 6, 2016

@clintwood - that's correct. This only introduces a syntax for expressing these kinds of directives/annotations in the schema language but offers no semantic meaning for them. Based on how each directive is intended to be used, I imagine some will allow defining metadata on a schema, some will allow defining decorator-style manipulations for the schema builder, some will allow defining client-side code generation artifacts, and I'm sure there are also future uses for them we haven't thought of yet.

@leebyron
Copy link
Contributor Author

leebyron commented May 6, 2016

@robzhu We won't revisit pre/postfix because doing so would be highly disruptive to all previously written queries and with marginal value. I think that decision sailed when we first introduced directives (albeit before open sourcing) into the language in a postfix notation.

Honestly I'm not too worried about the postfix notation - despite being different from some other languages it has proven to be an ergonomically valuable choice for Cap'n Proto, which was a major source of inspiration for this syntactic feature.

@leebyron leebyron merged commit 1b6824b into master May 6, 2016
@leebyron leebyron deleted the schema-directives branch May 6, 2016 21:56
sogko added a commit to sogko/graphql-js that referenced this pull request Jun 1, 2016
* master: (26 commits)
  0.6.0
  Validation: improving overlapping fields quality (graphql#386)
  Validation: context.getFragmentSpreads now accepts selectionSet rather than fragment AST node
  Factor out more closure functions
  Factor out closure functions to normal functions
  Deprecated directive (graphql#384)
  RFC: Directive location: schema definition (graphql#382)
  RFC: Schema Language Directives (graphql#376)
  Export introspection in public API
  Export directive definitions. (graphql#381)
  BUG: Ensure building AST schema does not exclude @Skip and @include (graphql#380)
  documentation of schema constructor
  Revert "Remove all 'instanceof GraphQLSchema' checks" (graphql#377)
  remove all 'instanceof GraphQLSchema' checks (graphql#371)
  Error logging for new interface type semantics (graphql#350)
  Nit: Missing case in grammar for TypeSystemDefinition in comment
  Bug: printer can print non-parsable value
  Factor out suggestion quoting utility
  Minor refactoring
  Minor refactoring of error messages for unknown fields
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants