[RFC] Schema Implementation API

[rmosolgo]

I want to provide a new API that is more familiar to Ruby developers. I hope that less DSL will make people more productive and happier 😀 See all the advantages and opinions discussed here: #727

The implementation is currently quite sloppy. But I've started writing docs and tests for what I want to build, so I'll be working towards that and cleaning up as I go. So, if anyone has feedback on those, please do share it!

[xuorig]

Have you thought of ways that could enable someone to collocate definition & implementation? My fear is the context switching between a very large .graphql file and smaller impl classes might be a bit hard. wdyt?

[cjoudrey]

from_definition currently accepts the IDL as an argument, was this meant to be File.read('app/graphql/my_schema.graphql') or were you thinking about making from_definition support paths?

[rmosolgo]

I merged this behavior separately in #872 👌

[cjoudrey]

Have you thought of ways that could enable someone to collocate definition & implementation?

That or adding some documentation to show how one could split one large schema.graphql into multiple smaller .graphql files. I suppose if from_definition accepts the IDL as a string, people could stitch the files together manually, but it might be useful to provide a simpler way.

[xuorig]

That or adding some documentation to show how one could split one large schema.graphql into multiple smaller .graphql files

That'd be great. HoweverI still think that if it was possible to quickly see a definition and an implementation without too much context switching it would be great!

[xuorig]

Have you thought of ways to extend the schema / implementation with common behaviour? Let's take my usual field :a, :string, preloads: b as an example. How would you implementsomething like a custom preloads argument with thisimplementation?

[cjoudrey]

Let's take my usual field :a, :string, preloads: b as an example. How would you implementsomething like a custom preloads argument with this implementation?

It would still be possible to do this in the resolver function, no?

i.e.

def a
  MyPreloader.preload(:b).then {
    # stuff
  }
end

Maybe just a bit more verbose, but that might not be a bad thing. 🤔

[xuorig]

Maybe just a bit more verbose, but that might not be a bad thing. 🤔

Hmm agreed but this might get very verbose and repetitive. We've got some fields that will

preload: { something: [ { something: something {] }, visibility: ..., filters: []

That would result in possibly huge resolvers, and having it directly in the resolver wouldnt let us use Instrumenters or other ways of reusing logic across fields / resolvers

[rmosolgo]

split one large schema.graphql into multiple smaller .graphql files

👍 to this, but I think it can be a separate task from this one. (Make .from_definition accept a glob and parse each matching file)

collocate definition & implementation?

I think it's interesting, but I'm scared of re-introducing traversal-based schema building. The current implementation has issues with Rails autoloading, cyclical dependencies, and modifying schema objects.

So, it's ok with me to start small (parse a single .graphql file) and build from there.

I can think of a few ways to collocate them which can be explored later, are there other approaches to consider?

• Ruby: make another DSL for describing GraphQL with Ruby

  class Blog 
    field :authors, { first: "Int", after: "String" }, "AuthorConnection"
    field :topics, "[Topic!]"
    # ... 
  end 

  Maybe the DSL could be better, but it's not exciting to me, I'd rather leverage the GraphQL IDL.

• Inline GraphQL, Ruby String: Put the GraphQL definition in the .rb file, as a string

  class Blog 
    structure <<-GRAPHQL 
      authors(first: Int, after: String): AuthorConnection
      topics: [Topic!]
    GRAPHQL 
  end 

• Inline GraphQL, Ruby Comment: Put the GraphQL definition in the .rb file, as a comment

  class Blog 
    =begin GRAPHQL 
    type Blog {
      authors(first: Int, after: String): AuthorConnection
      topics: [Topic!]
    }
    =end 
  end 

  We parse the file and extract the relevant bit. Could be buggy, but we can use the GraphQL IDL as-is, with no Ruby involved.

[rmosolgo]

Have you thought of ways to extend the schema / implementation with common behaviour?

I like your class-method based suggestions here and in your first issue, I think something like this would be ok:

# When accessing the posts field, preload the ratings
preloads posts: :ratings 
def posts(first:, after:) 
  # ... 
end 

I think we could implement this on top of ActiveSupport-like callbacks, for example:

before_resolve posts: :preload_ratings
def posts(first:, after:)
end 

to call methods before/after resolving. This would be a more human-friendly version of field instrumentation.

How does that sound?

[swalkinshaw]

# When accessing the posts field, preload the ratings
preloads posts: :ratings 
def posts(first:, after:) 
  # ... 
end

🤔 are you saying that instead of specifying all preloads at the top of the file, you just locate these above each method? Never thought of it and it solves a problem I had which came up in the other issue.

ie: before_resolve :whatever, only: %i(posts)

This works pretty well because the number of actions in a controller, or lifecycle callbacks in a model, are pretty limited. Some controllers could have a lot of actions, but most are CRUD based.

Using the same type of solution for GraphQL is a little different since it's pretty common to have a lot of fields which could quickly get out of control.

[rmosolgo]

My suggestion is a lot like controller hooks, just a different order of arguments:

before_resolve :posts, :do_thing 
# is like 
before_action :do_thing, only: :posts 

It could be anywhere in the class definition. It requires a lot of hackery to make it work without an explicit method name. A nice side-effect of the explicit name is that you could apply before_resolve to fields which don't have corresponding methods (that is, they're implemented by delegating to the proxied object).

[theorygeek]

This would probably be over-optimization at this point of development, but I wonder if eventually the call method here could be constructed at build-time by constructing the method as a string and then instance_eval'ing it. The idea is to avoid constructing the hash and doing this logic here - it may not even make a difference on performance, though.

Something like:

call_args = @graphql_arguments.map { |a| "#{a}: args[:#{a}]" }.join(', ')
# "letter: args[:letter]"

method_body = <<~RUBY
  def call(proxy, args, ctx)
    proxy.#{@method_name}(#{call_args})
  end
RUBY

# def call(proxy, args, ctx)
#   proxy.suit(letter: args[:letter])
# end

instance_eval(method_body)

[rmosolgo]

I'll definitely look into it, I had a similar TODO myself :)

If it's better on the benchmarks, I'll go for it!

[theorygeek]

So I'm thinking about :rest and :keyrest args:

# :rest
def my_resolver(*args)
  # => args == [{ :some_argument1 => 'some value 1' }]
end

# :keyrest
def my_other_resolver(**kwargs)
  # => kwargs == { :some_argument1 => 'some value 1' }
end

A couple of thoughts on this:

• It seems like we would want to support those kinds of arguments. And if they exist, we'd basically only check to make sure that there are no positional args, and no unknown keyword args.
• Thinking through that case, if a user did use *args or **kwargs, I'm not sure whether they would expect to get the special args or not.
• It seems like it'd be most predictable if we could guarantee that every call into a resolver method would be done with the exact same signature. But that would require us to take a stance that either all methods will accept :ast_node & friends, or no methods would accept them.

That third point is really about avoiding brittle scenarios, where changing from seemingly identical method signatures ends up changing behavior. For example:

def my_resolver(some_arg1:, irep_node:, ast_node:)
end

# Several months later, someone refactors this into:
def my_resolver(**kwargs)
end

Perhaps the best option right now would be to raise an InvalidImplementationError upon encountering a :rest or :keyrest parameter.

[rmosolgo]

raise an InvalidImplementationError upon encountering a :rest or :keyrest parameter.

👏

[swalkinshaw]

Implementation isn't working right now since it's hardcoded to nil here

[rmosolgo]

I'm not actively working on this branch. I decided to try experimenting with GitHub's schema a bit to see if I can find something that would work, then try it out a bit before upstreaming it.

I'm worried about building something from scratch, then releasing it & applying it, but learning that it doesn't really solve our problems 😖