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

Extensible Printer #791

Closed
minasmart opened this issue Jun 19, 2017 · 4 comments
Closed

Extensible Printer #791

minasmart opened this issue Jun 19, 2017 · 4 comments

Comments

@minasmart
Copy link

In an attempt to use custom directives for annotation, I made an attempt to extend the IDL printer. I couldn't find a way to do that didn't involve a monkey patch. The module nesting and composition makes it really hard to break into the underlying printers.

In my case, here's what I had to do to expose one more directive on a field definition:

# frozen_string_literal: true
module GraphQL
  class Schema
    class Printer
      module TypeKindPrinters
        # This module was added
        module AccessRestrictedPrinter
          def print_access_restricted(field)
            return unless field.metadata[:access_restricted]

            case field.metadata[:access_restricted]
            when true
              " @accessRestricted"
            else
              " @accessRestricted(reason: #{field.metadata[:access_restricted].to_s.inspect})"
            end
          end
        end

        module FieldPrinter
          include AccessRestrictedPrinter
          def print_fields(warden, type)
            fields = warden.fields(type)
            fields.map.with_index do |field, i|
              # This is the only line I had to change from the source
              "#{print_description(field, '  ', i == 0)}"\
              "  #{field.name}#{print_args(warden, field, '  ')}: #{field.type}#{print_deprecated(field)}#{print_access_restricted(field)}"
            end.join("\n")
          end
        end

        # The two classes below needed to re-extend to gain access to the AccessRestrictedPrinter
        class ObjectPrinter
          extend FieldPrinter
        end

        class InterfacePrinter
          extend FieldPrinter
        end
      end
    end
  end
end

Would it be possible to refactor the printer to make it more extensible?

@rmosolgo
Copy link
Owner

Hi! I suppose the current structure is not very open for customization 😅 Thanks for sharing your use case, that really helps me see what you have in mind.

I think this is also part of the conversation about how to make better use of the GraphQL IDL (eg, #727, #789 ).

Here's a possibility I can think of:

  • Improve the printer to render all directives, not just @deprecated
  • Add a step to printing where you can add directives based on the field

That way, you could implement a function for that pre-printing step to assign your directive based on the field's configuration, then it would be printed out just like @deprecated. What do you think of that?

One issue from the IDL discussion is relevant here: by outputting custom directives, are we breaking compatibility with other GraphQL libraries?

@minasmart
Copy link
Author

It sounds great to me! And I don't think outputting custom directives in the IDL will interfere with clients? Most use the JSON output, which, even though directives can exist on any node, does not allow them to be introspected, hence the weird handling of deprecation in the JSON introspection response.

I feel like there's definitely a bigger conversation to be had about how directives are exposed through introspection.

The following query

query { __type(name: "__Field") { fields { name } } }

Returns

{
  "data": {
    "__type": {
      "fields": [
        {
          "name": "args"
        },
        {
          "name": "deprecationReason"
        },
        {
          "name": "description"
        },
        {
          "name": "isDeprecated"
        },
        {
          "name": "name"
        },
        {
          "name": "type"
        }
      ]
    }
  }
}

At some point, it may be great for the __Field type (and likely other nodes) to expose directives as an introspectable field.

@rmosolgo
Copy link
Owner

rmosolgo commented Jan 26, 2018

There's now a base GraphQL::Language::Printer class which can be extended for custom printer, please give a try and open an issue if you find any trouble with it!

https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/language/printer.rb

@minasmart
Copy link
Author

This looks awesome! Thank you!

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

No branches or pull requests

2 participants