[1.8] Try again: interfaces as Ruby modules

[rmosolgo]

Another take on #1286

• What about unions? Keep them as classes?

Before (.define)

This is the previous way to make an interface:

Commentable = GraphQL::InterfaceType.define do 
  name "Commentable" 
  field :comments, types[Comment], resolve: -> (obj, args, ctx) { 
    obj.comments.moderated 
  }
end 

After (class)

This is the current way on 1.8-dev:

class Commentable < GraphQL::Schema::Interface 
  field :comments, [Comment], null: false 
  
  module Implementation 
    def comments 
      @object.comments.moderated 
    end 
  end 
end 

After (module)

This is what I'm exploring/proposing in this branch

module Commentable 
  include GraphQL::Schema::Interface 

  field :comments, [Comment], null: false 
  def comments 
    @object.comments.moderated 
  end 
end 

The main difference is that the module Implementation is not required to customize field resolution. As a benefit, the implementation of the field sits right next to the field signature.

[rmosolgo]

I got a great question on Slack so I wrote the response here for posterity:

rmosolgo [2:44 PM]
Man, the big difficulty with interfaces
i had the same thing in the gem, now in the app
is inheritance of class methods
with classes, it just works
but with modules
not good
so the url_field macro above no worky

theorygeek [2:45 PM]
what about ActiveSupport::Concern’s class_methods pattern?

Regarding AS::C, I took a crack at it, even porting most of it to GraphQL::Schema::Member::Concern.

But the problem is, AS::C treats modules in one of two different ways:

• Either, the module is another concern, in which case, class methods from its parent concerns are not mixed in;
• Or, the module is not a concern, so class methods are mixed in, but then AS::C's inheritance magic stops there.

Here's the source: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/concern.rb#L114

The problem here is that we need a mix. For example, this doesn't work:

module BaseInterface 
  include GraphQL::Schema::Interface 
  field_class MyCustomField 
  
  # A helper for defining fields:
  def self.field_macro(...); end 
end

module Commentable 
  include BaseInterface 

  field_macro(...)
end 

It doesn't work because Commentable doesn't get self.field_macro from BaseInterface.
Since BaseInterface wasn't a concern, all the class methods were dumped there, but
modules that include it don't get any special help.

So, what if you made BaseInterface into a Concern?

module BaseInterface 
  extend GraphQL::Schema::Member::Concern
  include GraphQL::Schema::Interface 
  field_class MyCustomField 
  
  # A helper for defining fields:
  def self.field_macro(...); end 
end

module Commentable 
  include BaseInterface 

  field_macro(...)
end 

Then you get undefined method field_class, because, since BaseInterface was a concern, it didn't get class methods from its ancestors.

So, how can you get both macros from upstream and continue sending them downstream? I really fiddled with this a lot, tweaking various included hooks
and ClassMethods modules, but I couldn't find a solution.

[theorygeek]

One little nit on the syntax, is extend a better choice?

module Commentable 
  extend GraphQL::Schema::Interface

(On an ActiveSupport::Concern, I think you use include to add functionality to your class and extend to add functionality to your module. I might be wrong though.)

[rmosolgo]

Another good question was: how about getting rid of the module Implementation altogether? Something like:

class MyInterface < GraphQL::Interface
  field :foo
  def foo; end
end

The thing I couldn't figure out here was, how to get a self that made sense for the body of that method. I think best would be if it was included into the class just like a module's method, but I couldn't find any way to do that. There are a lot of ways to grab methods, but you can't really put them on other classes:

irb(main):001:0> class C1; def m1; end; end
=> :m1
irb(main):002:0> class C2; end
=> nil
irb(main):003:0> method = C1.instance_method(:m1)
=> #<UnboundMethod: C1#m1>
irb(main):004:0> method.bind(C2)
Traceback (most recent call last):
        3: from /Users/rmosolgo/.rbenv/versions/2.5.0/bin/irb:11:in `<main>'
        2: from (irb):4
        1: from (irb):4:in `bind'
TypeError (bind argument must be an instance of C1)
irb(main):005:0> method.bind(C2.new)
Traceback (most recent call last):
        3: from /Users/rmosolgo/.rbenv/versions/2.5.0/bin/irb:11:in `<main>'
        2: from (irb):5
        1: from (irb):5:in `bind'
TypeError (bind argument must be an instance of C1)

So, I'm not sure how to execute that method in the context of the runtime object.

There's another option which I have not explored: perhaps interface fields could be called on an instance of the interface. That is, whenever an interface is requested, you'd initialize an instance of the Interface type, and call methods on it.

I'm afraid it would be pretty weird, because, for example, ivars would not be shared between methods, the way they are between ruby modules. Also, it would get tricky if objects override their interface fields in some way. For example, one or more of: re-call field, redefine the method. 😬

[theorygeek]

Does it break anything if I make an interface module, and do something like:

module SomeInterface
  # what if I flip these two around?
  extend ActiveSupport::Concern
  include GraphQL::Schema::Interface
end

[rmosolgo]

Regarding extend vs include, what if you want to inherit field implementations, for example:

module AbstractNameable 
  extend_or_include GraphQL::Schema::Interface 

  field :name, String, null: false 
  def name 
     @object.display_name 
  end 
end 

module Actor 
  extend_or_include AbstractNameable
end 

class Person < GraphQL::Schema::ObjectType
  implements AbstractNameable 
end 

Somehow, we want Person#name to go to AbstractNameable#name, right? I think we'll get that for free if we use include.

[rmosolgo]

Ok, I just force-pushed this branch with a much improved implementation, in my opinion. I'm pretty happy with the implementation.

Thanks @theorygeek for talking this over with me in Slack, after clarifying exactly what we need (inheritance of instance methods; inheritance of class methods) I got a better sense of what implementation would work for us. So, now it works mostly like this:

• GraphQL interfaces include GraphQL::Schema::Module (or another interface type)
• GraphQL objects implement(SomeInterface)
• Class methods go in the ClassMethods module (TODO: should I choose another name to avoid possible naming conflicts, for example, with AS::Concern?)
• Instance methods are inherited in the normal way
• Class methods are inherited by extend(parent::ClassMethods) in each child interface

I think this is really nice API and a reasonably sane implementation 😊

TODO:

• Update the upgrader
• Rename ClassMethods -> DefinitionMethods?

[rmosolgo]

All of these were extracted so I could keep Schema::Member as a base class, but easily get these methods into Schema::Interface.

[rmosolgo]

Here was a weird naming conflict with ClassMethods, case in point for renaming Schema::Interface's inherited module to DefinitionMethods

[rmosolgo]

TODO: this should only set it if it's not already defined

[rmosolgo]

(This was just a document-how-it-actually-works test, I think the new behavior is better anyway)

[rmosolgo]

 java.lang.NullPointerException: null

😱 😱 😱

[theorygeek]

It is looking pretty nice! I think I was hoping that you would do:

module MyInterface
  extend GraphQL::Schema::Interface
end

class MyObjectType < GraphQL::Schema::Object
  include MyInterface
end

Because then you get these semantics, which seem nice:

# Is this type an Interface type?
MyInterface.is_a?(GraphQL::Schema::Interface)
# => true

# Is this type an Interface type?
MyObjectType.is_a?(GraphQL::Schema::Interface)
# => false

# Test for interface implementation
MyObjectType < MyInterface
# => true

# Test if the object instance supports that interface
MyObjectType.new.is_a?(MyInterface)
# => true

However... it is probably overrated, and perhaps even a bit confusing. Because right now, you would test if a type is an object type using:

MyObjectType < GraphQL::Schema::Object
# => true

[rmosolgo]

Yeah, I can't think of a way to make it consistent! I think I'm going to merge this branch soon, if you want to experiment with the different hooks, I'm open to switching it around a bit, but I'm not optimistic I can make it drastically better than it is now.

[theorygeek]

I wrote up a little gist that had a real basic example of how it might work:
https://gist.github.com/theorygeek/9f4f68980f0c0dd89b116ad8ac0e7acc

TBH... I am not sure that they are huge improvements. I think the nicest thing is that it solves:

• MyObjectType < GraphQL::Schema::Object is true
• MyObjectType < GraphQL::Schema::Interface is falsey

[rmosolgo]

Thanks for sharing those notes, I took a try at it here:

interface-as-module...interface-as-module-extends

I'm not quite sure it's worth it because:

• inheriting methods doesn't Just Work anymore. We could include them modules during the extended hooks, but that defeats the purpose, right?
• Although MyObj < GraphQL::Schema::Object works, MyInterface < GraphQL::Schema::Object doesn't work (in either case, afaik) , so i don't think it's really worth investing in < to check object type. Maybe we can add a different class method for checking that.

[rmosolgo]

also, I added definition_methods do ... end in 56c6a14, thanks for the tip!

[rmosolgo]

I'm gonna drop JRuby from the build. It passes locally. I'll open a failing PR with the issue (and I've reported it to JRuby here: jruby/jruby#3680 (comment)