Skip to content

Default resolver for interface fields #130

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

Closed
theorygeek opened this issue Apr 1, 2016 · 8 comments
Closed

Default resolver for interface fields #130

theorygeek opened this issue Apr 1, 2016 · 8 comments

Comments

@theorygeek
Copy link
Contributor

In #108, we addressed the "mixin" type functionality that graphql-ruby was missing by allowing interfaces to provide default implementations for fields. This is an incredibly useful feature, and allows an interface to function similarly to how modules function in plain Ruby; I try to do this with all of my interfaces.

However, what that also means is that if you don't provide an implementation for a field that comes from an interface, you'll get the default behavior, which is to do public_send against the underlying object.

In my experience, this is almost never the correct behavior, and usually means that I simply forgot that the ObjectType I was creating needs to define that field. Many of the fields defined in my interfaces require more complicated resolver methods, and often the names of the methods are different anyway (camelCase vs snake_case).

So I think it might be good to require the developer to "opt-in" to receiving the default field implementations from interfaces, but that may be controversial, so I thought I'd just ask in an issue before doing a PR.

Possible Solutions
One solution would be to modify GraphQL::ObjectType so that it would only include fields from an interface if the interface was not using a default resolver. For example, the developer would have to define resolve proc, or specify a property: or field:, etc. If they did not do this, it would not be returned by #get_field, or #all_fields, etc.

Another solution would be to change the default implementations for fields defined on interfaces to throw errors, although the drawback is that you wouldn't find out that you forgot to define a field until runtime.

A third solution would be to add some sort of flag to the field when it's defined in the interface, such as inherit: true, and only these fields would be copied to the object type.

What do you guys think?
/cc: @rmosolgo @xuorig @peterhorne

@rmosolgo
Copy link
Owner

rmosolgo commented Apr 1, 2016

I'm starting to sound like a broken record on this "accepts_definitions" API but here I go again!

Here's an option you could use (or we could add to this gem):

# Define a callable definition
module AbstractField 
  def self.call(interface_type, field_name, return_type)
    field_name_s = field_name.to_s 
    abstract_field = GraphQL::Field.define do 
      name(field_name_s)
      type(return_type)
      resolve -> (obj, args, ctx) { raise(NotImplementedError, "Can't resolve #{field_name_s} on #{obj}; resolution not defined" }
    end 
    interface_type.fields[field_name_s] = abstract_field
  end
end

# Attach it to InterfaceType
GraphQL::InterfaceType.accepts_definitions(abstract_field: AbstractField) 

# Use it in a definition
MyInterface = GraphQL::InterfaceType.define do 
  # ... 
  abstract_field :something, types.String # will raise NotImplemented if accessed
end

This actually isn't quite right -- we want it to 💥 at load-time, not runtime when someone wants to use it... but that might be a start!

@rmosolgo
Copy link
Owner

One thing that needs an overhaul is Schema's build-time validation. Right now the implementation is obtuse and it's missing some crucial features (validating interface implementation comes to mind :S). Maybe that can be redone in the same way .define was redone, so that it applies a collection of rules. Then, the library could come with a bunch of default rules, but you could add your own too.

@rmosolgo
Copy link
Owner

rmosolgo commented Dec 6, 2016

FWIW I'd still like to support this, something like

field :name, abstract: true 

The choice to inherit by default might not have been the right one!

@rmosolgo
Copy link
Owner

rmosolgo commented Dec 6, 2016

I just left this open because I hadn't actually done it yet! 😅

@theorygeek
Copy link
Contributor Author

Oh no worries... I was honestly just cleaning up. I'll open it back up, that's a great suggestion!

@rmosolgo
Copy link
Owner

rmosolgo commented Mar 7, 2017

Removing inheritance-by-default with the implements method in #574

@rmosolgo
Copy link
Owner

rmosolgo commented Mar 9, 2017

This will be available via implements in 1.5.0 !

@rmosolgo rmosolgo closed this as completed Mar 9, 2017
@theorygeek
Copy link
Contributor Author

Nice, thanks! 🎉

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