Add schema language directives #273
Conversation
There was a problem hiding this comment.
🎉 Surprise, everything can have directives now! This is awesome, thanks for making it happen. I forgot it would require getting all the way down to the parser 😬
As far as the breaking change to the Nodes API, it's safe to make as far as I know. I haven't heard of any third-party use of the Schema AST.
Just a couple of questions above to make sure I understand the "whys" before adding some code.
| @@ -202,7 +202,7 @@ class FragmentDefinition < AbstractNode | |||
| def initialize_node(name: nil, type: nil, directives: [], selections: []) | |||
| @name = name | |||
| @type = type | |||
| @directives = directives | |||
| @directives = directives || [] | |||
There was a problem hiding this comment.
I see that directives is often falling back to || [] -- what's the case where this is needed? (I ask because I'm only aware of directives_list_opt in the parser, which already provides [] for the empty case.)
There was a problem hiding this comment.
Good question. I did this because I saw we had done it for ObjectTypeDefinition interfaces.
I was under the impression we did this so that if someone intentionally passes nil, it would get turned into []. It does add quite a bit noise, maybe we can just remove the || [].
There was a problem hiding this comment.
👍 I'd say if the tests are green without this, then let's not add them. If "people" pass nil, they get nil :P (fortunately I think "people" is just parser.rb)
| @@ -116,7 +130,7 @@ def generate(node, indent: "") | |||
| when Array | |||
| "[#{node.map { |v| generate(v) }.join(", ")}]" | |||
| when Hash | |||
| "{ #{node.map { |k, v| "#{k}: #{generate(v)}" }.join(", ")} }" | |||
| "{#{node.map { |k, v| "#{k}: #{generate(v)}" }.join(", ")}}" | |||
There was a problem hiding this comment.
Am I right that this whitespace change was added so that our dump matches the "kitchen sink" example from graphql.js ? It's good by me, I just want to make sure I understand it before adding noise to peoples' diffs :P
There was a problem hiding this comment.
That's correct. I took for granted that Facebook's "kitchen sink" was formatted correctly. Also: https://github.com/graphql/graphql-js/blob/a725499b155285c2e33647a93393c82689b20b0f/src/language/printer.js#L81
cjoudrey
force-pushed
the
add-directives-parsing
branch
from
9c26834 to
ef68514
Compare
|
Awesome, thanks for this new feature! |
Back in May, graphql/graphql-js#376 added support for directives in GraphQL IDL.
Deprecations were added as directives to the IDL in graphql/graphql-js#384.
Finally, descriptions were added to the IDL in graphql/graphql-js#464.
This is part 1 of a few pull requests I will be submitting that will add all of these features to the gem.
This pull request includes:
schema-kitchen-sink.graphqlto ensure we are consistent.EnumValueDefinitionAST node to support directives on enum values. This is a breaking API change asGraphQL::Language::Nodes::EnumTypeDefinition#valuesused to return an array of strings, now it will return an array ofEnumValueDefinition.👀 @rmosolgo @dylanahsmith