Add deprecations to IDL #275

cjoudrey · 2016-09-23T17:28:16Z

This is part 2 of #273 and is currently based off it.

This PR adds support for deprecations in the IDL using directives as per graphql/graphql-js#384.

Part 3 will be adding descriptions to IDL as per graphql/graphql-js#464.

rmosolgo · 2016-09-23T22:11:27Z

lib/graphql/directive.rb

    ]

    def initialize
      @arguments = {}
    end

    def include?(arguments)
+      return true if include_proc.nil?


The bad news is, I caused a conflict.

The good news is, this is implemented directly in DirectiveChecks and this method is gone 🎉

cjoudrey · 2016-09-23T22:19:25Z

Haha don't worry! Saw it coming. :D I'll fix tonight.

On Friday, 23 September 2016, Robert Mosolgo [email protected]
wrote:

@rmosolgo commented on this pull request.

In lib/graphql/directive.rb
#275 (review)
:
 ]
 def initialize
   @arguments = {}
 end

 def include?(arguments)
 return true if include_proc.nil?
The bad news is, I caused a conflict.

The good news is, this is implemented directly in DirectiveChecks and
this method is gone 🎉

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#275 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXg9seGIj0tclHJdyFpPYqsPpyTWUQfks5qtE6QgaJpZM4KFNa8
.

cjoudrey · 2016-09-24T04:11:04Z

@rmosolgo conflict solved. 🎉

cjoudrey · 2016-09-25T21:03:06Z

Any other comments for this one? :)

On Friday, 23 September 2016, Christian Joudrey [email protected] wrote:

Haha don't worry! Saw it coming. :D I'll fix tonight.

On Friday, 23 September 2016, Robert Mosolgo <[email protected]
javascript:_e(%7B%7D,'cvml','[email protected]');> wrote:
@rmosolgo commented on this pull request.

In lib/graphql/directive.rb
#275 (review)
:
 ]
 def initialize
   @arguments = {}
 end

 def include?(arguments)
 return true if include_proc.nil?
The bad news is, I caused a conflict.

The good news is, this is implemented directly in DirectiveChecks and
this method is gone 🎉

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#275 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXg9seGIj0tclHJdyFpPYqsPpyTWUQfks5qtE6QgaJpZM4KFNa8
.

rmosolgo

Yes, sorry I cooked up some half-thoughts and hadn't made time to turn them into whole thoughts. In fact... they're still only half-thoughts, but I put them here anyways 😬 what do you think?

rmosolgo · 2016-09-26T15:50:57Z

lib/graphql/schema/printer.rb

+              ' @deprecated'
+            else
+              " @deprecated(reason: #{field_or_enum_value.deprecation_reason.to_s.inspect})"
+            end


To make sure I understand, is this equivalent to three, mutually-exclusive branches, ie

case field_or_enum_value.deprecation_reason when nil "" when "" " @deprecated" else " @deprecated(...)" end

? I guess there's a different behavior with false, but I don't think we expect false here, right ?

It looks like the JS version handles the "default deprecation reason" specially, should we do the same?

https://github.com/graphql/graphql-js/pull/384/files#diff-71ba52e9c625f826d2b0df2963c8633aR170

(eg case "", GraphQL::Directive::DEFAULT_DEPRECATION_REASON ?)

Good catch. The code is clearer with the case statement. I'll make that change.

rmosolgo · 2016-09-26T15:54:24Z

lib/graphql/schema/printer.rb

@@ -44,6 +48,10 @@ def print_schema_definition(schema)
      BUILTIN_SCALARS = Set.new(["String", "Boolean", "Int", "Float", "ID"])
      private_constant :BUILTIN_SCALARS

+      def is_spec_directive(directive)
+        ['skip', 'include', 'deprecated'].include?(directive.name)
+      end


I'm trying to figure out how this compares to the JS implementation, are there ever cases that we don't want to print these out?

Good question.

I ported this from the JS implementation: https://github.com/graphql/graphql-js/blob/5e3d377e6e0881191efe02b9d40720ff0f0b5b0f/src/utilities/schemaPrinter.js#L28-L44

My understanding of the JS code is that when printing the schema, we do not want to include spec directives since they are part of the spec. We still print non-spec directives though.

When printing the introspection query, we only include spec directives.

non-spec directives

Part of me wants to leave this out, since there aren't any non-spec directives yet. But ... if we left it out, I can guarantee people would be surprised not to see @defer / @stream in the query dump if I ever got around to merging that 😬

would be surprised not to see @defer / @stream in the query dump

Just to make sure we're on the same page. You'd still see @defer and @stream on the fields, you just don't see their definitions, i.e. directive @defer.

I think the other reason they do this in the JS implementation is because directives can be specified on the schema: https://github.com/graphql/graphql-js/blob/a725499b155285c2e33647a93393c82689b20b0f/src/type/schema.js#L50-L54

We don't support that yet.

You're right, maybe I don't understand! If graphql-ruby supports @stream and @defer, but they aren't part of the GraphQL spec, would they show up in a schema print-out or not?

Don't worry, I kept confusing myself here too.

There's two concepts: Directive and DirectiveDefinition.

Example @skip Directive:

query myQuery($someTest: Boolean) { experimentalField @skip(if: $someTest) }

Example @skip DirectiveDefinition:

directive @skip(if: Boolean!) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT

The is_spec_directive method is used to omit the DirectiveDefinition of spec directives from getting printed when dumping the schema.

I need to rename def print_directive(directive) to def print_directive_definition(directive) as that's not named correctly and could be a source of confusion.

In terms of Directive we're only printing the deprecated directive at the moment, see DeprecatedPrinter. We may want to generalize this in the future though.

Edit: So to answer your question about @stream and @defer, if we don't add them to is_spec_directive their definitions will get printed, but if you use GraphQL::Language::Generation to print a document representing a query that uses @defer on some selection set, it won't show up unless we print it in GraphQL::Language::Generation.

rmosolgo · 2016-09-27T14:00:49Z

spec/graphql/schema/printer_spec.rb

@@ -14,6 +14,7 @@
      value "FOO"
      value "BAR"
      value "BAZ", deprecation_reason: 'Use "BAR".'
+      value "WOZ", deprecation_reason: GraphQL::Directive::DEFAULT_DEPRECATION_REASON


rmosolgo · 2016-09-27T14:03:48Z

🤘 Very cool, thanks!

rmosolgo reviewed Sep 23, 2016

View reviewed changes

Add deprecations to IDL

30d65d3

cjoudrey force-pushed the add-deprecations-to-idl branch from 2553953 to 30d65d3 Compare September 24, 2016 04:10

rmosolgo reviewed Sep 26, 2016

View reviewed changes

Add support for DEFAULT_DEPRECATION_REASON

e0f8f37

rmosolgo reviewed Sep 27, 2016

View reviewed changes

rmosolgo merged commit 93ce300 into rmosolgo:master Sep 27, 2016

cjoudrey mentioned this pull request Sep 28, 2016

Parity with graphql-js' introspection schema #284

Merged

rmosolgo modified the milestone: 0.19.0 Sep 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add deprecations to IDL #275

Add deprecations to IDL #275

cjoudrey commented Sep 23, 2016 •

edited

Loading

rmosolgo Sep 23, 2016

cjoudrey commented Sep 23, 2016

@rmosolgo commented on this pull request.

cjoudrey commented Sep 24, 2016

cjoudrey commented Sep 25, 2016

@rmosolgo commented on this pull request.

rmosolgo left a comment

rmosolgo Sep 26, 2016

rmosolgo Sep 26, 2016 •

edited

Loading

cjoudrey Sep 26, 2016

rmosolgo Sep 26, 2016

cjoudrey Sep 26, 2016

rmosolgo Sep 27, 2016

cjoudrey Sep 27, 2016

rmosolgo Sep 27, 2016

cjoudrey Sep 27, 2016 •

edited

Loading

rmosolgo Sep 27, 2016

rmosolgo commented Sep 27, 2016

Add deprecations to IDL #275

Add deprecations to IDL #275

Conversation

cjoudrey commented Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

cjoudrey commented Sep 23, 2016

@rmosolgo commented on this pull request.

cjoudrey commented Sep 24, 2016

cjoudrey commented Sep 25, 2016

@rmosolgo commented on this pull request.

rmosolgo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmosolgo Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjoudrey Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmosolgo commented Sep 27, 2016

cjoudrey commented Sep 23, 2016 •

edited

Loading

rmosolgo Sep 26, 2016 •

edited

Loading

cjoudrey Sep 27, 2016 •

edited

Loading