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

Release 1.8.0 #1389

Closed
14 tasks done
rmosolgo opened this issue Apr 4, 2018 · 11 comments
Closed
14 tasks done

Release 1.8.0 #1389

rmosolgo opened this issue Apr 4, 2018 · 11 comments
Milestone

Comments

@rmosolgo
Copy link
Owner

rmosolgo commented Apr 4, 2018

This issue is to track the stable release of 1.8.0

TODO

Roughly in order:

@rmosolgo rmosolgo added this to the 1.8.0 milestone Apr 4, 2018
@hmans
Copy link

hmans commented Apr 5, 2018

Hey @rmosolgo, first of all, thank you for your work on graphql-ruby!

We're using it in a new project, and I've been wondering: should we start with the current pre-release of 1.8.0 right away, or should we stick to 1.7.x and wait until the first RC or so? Thanks!

@rmosolgo
Copy link
Owner Author

rmosolgo commented Apr 5, 2018

If it were me, I'd start on 1.8. The only big caveat is Interface types -- I might switch up the current implementation so that they're Ruby modules. So you could handle that by either writing them as classes and taking the hit to redo them if they change, or writing them in the old .define style, and updating them later.

But the usability of the class-based style is so nice, I'd recommend it!

@hmans
Copy link

hmans commented Apr 5, 2018

Our project isn't going to be terribly large, so a certain amount of refactoring to keep up with 1.8's development is perfectly acceptable. Thanks for the heads-up!

@jcbpl
Copy link

jcbpl commented Apr 11, 2018

@rmosolgo I'll echo the thanks for all your hard work, the 1.8 changes are really great.

I'm starting to migrate a project over to the class-based API, and I'm trying to decide on if/how to change the way we're dynamically generating types. I think there are a number of ways it could be done (including leveraging #to_graphql), but I'm wondering if you've had any ideas yet about what an approach that doesn't use GraphQL::ObjectType.define might look like, or if that's even worth trying right now.

@rmosolgo
Copy link
Owner Author

rmosolgo commented Apr 11, 2018

I think it will be Class.new(BaseObject) { graphql_name(generated_type_name) ... }, what do you think of that?

Edit: there are some trivial and incomplete examples in the specs, for example:

new_object_class = Class.new(object_class) do
field :newField, String, null: true
field :name, String, description: "The new description", null: true
end

# Manually assign a name since `.name` isn't populated for dynamic classes
new_subclass_1 = Class.new(object_class) do
graphql_name "NewSubclass"
end

@jcbpl
Copy link

jcbpl commented Apr 12, 2018

Makes sense! We also add fields dynamically, and I initially thought I might be able to use class_eval to add the resolve method, but I'm not sure if there's a way to get the dynamically-created class in the Field instance. Here's what I'm playing with at the moment:

Factory:

class Graph::Factories::Metric
  def self.define(metric)
    if Graph::Objects::Interval.fields[metric.source].nil?
      return_type = Graph::Objects::PostSource.create(metric.source)

      field = Graph::Fields::PostSource.new(metric.source, return_type, 
        owner: Graph::Objects::Interval, null: false)

      Graph::Objects::Interval.add_field(field)
    end
  end
end

Field:

class Graph::Fields::PostSource < Graph::Fields::Base
  def initialize(*args, **kwargs, &block)
    source = args.first
    resolve = -> (obj, args, ctx) { obj.where(source: source) }

    super(*args, resolve: resolve, **kwargs, &block)
  end
end

Return Type:

class Graph::Objects::PostSource < Graph::Objects::Base
  def self.create(source)
    Class.new(self) do
      graphql_name "#{source.classify}Posts"
    end
  end

  field :count, Integer, null: false

  def count
    @object.count
  end
end

So the one part still relying on the pre-1.8 API is the resolve proc. I suppose I could call class_eval on the return type class directly in the factory, but subclassing field this way and defining resolve there made more intuitive sense to me.

@rmosolgo
Copy link
Owner Author

Could you share an example of how Graph::Factories::Metric.define is used? Just curious to get a sense of it.

@jcbpl
Copy link

jcbpl commented Apr 12, 2018

Right now I'm just calling it from Graph::Objects::Interval (Metric is an AR model):

class Graph::Objects::Interval < Graph::Objects::Base
  Metric.all.each do |metric|
    Graph::Factories::Metric.define(metric: metric)
  end
end

Ideally though I think I want to be able to call .define when I create Metric record(s) in a test, and have the fields and types be added so that I could verify them in a GraphQL test. I also probably want to call it from an initializer (rather than from Interval) in dev and prod.

@rmosolgo
Copy link
Owner Author

Thanks for sharing. I tried to understand it by inlining a lot of the code. Then I made a few changes:

  • Remove def count since the implementation there is the default behavior
  • Use define_method instead of a resolve proc
  • Instead of initializing a Field instance and using add_field, use the field(...) helper

In my case it turned out looking like this:

class Graph::Objects::Interval < Graph::Objects::Base
  # Read some objects from the database 
  Metric.all.each do |metric|
    # Generate a field from each object, using `source` as the name 
    field_name = metric.source
    # Don't override an existing field 
    if fields[field_name].nil?
      # Generate a return type for this field. It has a `count` field.
      return_type = Class.new(Graph::Objects::Base) do
        graphql_name "#{field_name.classify}Posts"
        field :count, Integer, null: false
      end

      # Define a field with the same name, returning the generated return type 
      field(field_name, return_type, null: false)

      # Implement the field with a method 
      define_method(field_name) do 
        @object.where(source: field_name)
      end 
    end
  end
end

Like you said, maybe for your app you want to reorganize it a bit to fit your flow, but I thought it was a fun exercise and maybe a fresh perspective would come in handy! Thanks again for sharing your use case a bit.

@jcbpl
Copy link

jcbpl commented Apr 12, 2018

Thanks! The inline approach is pretty much where I started. 😄

I just realized I could use irep_node to infer the field name in a resolve method directly on Interval:

class Graph::Factories::Metric
  def self.define(metric:)
    field_name = metric.source

    if Graph::Objects::Interval.fields[field_name].nil?
      return_type = Graph::Objects::PostSource.create(field_name)
      Graph::Objects::Interval.field(field_name, return_type, null: false,
        method: :resolve_post_source, extras: [:irep_node])
    end
  end
end

class Graph::Objects::PostSource < Graph::Objects::Base
  def self.create(source)
    Class.new(self) do
      graphql_name "#{source.classify}Posts"
    end
  end

  field :count, Integer, null: false
end

class Graph::Objects::Interval < Graph::Objects::Base
  def resolve_post_source(irep_node:)
    object.where(source: irep_node.name)
  end
end

I'm pretty happy with this approach—no need for separate Fields, and it eliminates any potential confusion around what's in scope in define_method or class_eval, since the resolver in the usual place.

I think the only slight improvement might be a way to get field_name without using irep_node... extras might be the only thing that feels slightly out of place to me in the new API.

@rmosolgo
Copy link
Owner Author

Cool! The only thing I would say is to use irep_node.definition_name instead. If the query uses a field alias, then .name will return the alias, not the real field name. extras: is a bit of a hack ... but it'll do for now 😆

@rmosolgo rmosolgo closed this as completed Aug 1, 2018
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

3 participants