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

Add a schema-level hook for lazy/batched resolution #1784

Merged
merged 4 commits into from
Sep 14, 2018

Conversation

rmosolgo
Copy link
Owner

@rmosolgo rmosolgo commented Aug 15, 2018

GraphQL-Ruby's lazy-resolve system is OK, but it has a high overhead without much benefit from its generality. Most of the time, a schema has only one type of lazy object (batch-loader or graphql-batch), and the rest of the time, all the method lookup stuff is overcomplicated and a waste of time.

So, what if we exposed some schema-level methods that could be overridden as needed? In fact, we've already done this in GitHub as a performance consideration (we overrode #lazy_method_name).

TODO:

  • test that overriding works

cc @exAspArk

@exAspArk
Copy link
Contributor

exAspArk commented Aug 31, 2018

👍

Just as an idea, here is how I personally imagine API could look like:

class PostType < GraphQL::Schema::Object
  field :id, ID, null: false
  field :comments, [CommentType], null: true, lazy: true
  #                                           ^
  # an explicit flag, no more matching by predefined classes to detect lazy objects
  # duck typing, yay!
end

class Schema < GraphQL::Schema
  query QueryType

  def lazy_resolve(value)                # or any other new API method with a default implementation, can be overridden
    case value
    when MyBatchClass
      value.sync                         # existing behavior, sort of
    else
      value.resolve_all if value.quacks? # custom behavior if necessary
    end
  end
end
  • Potentially no breaking changes
  • Can still support different lazy classes and methods within a schema
  • Can be extended to detect lazy objects not only by class

@rmosolgo
Copy link
Owner Author

@exAspArk , I think the difference between the code on this branch is the addition of lazy: true, is that right?

I don't think I want to add field-level opting in, because we have various execution wrappers that may or may not wrap eager values in lazy objects. So, it can be hard to know which fields return lazy values under which conditions!

@rmosolgo rmosolgo added this to the 1.8.9 milestone Sep 13, 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

Successfully merging this pull request may close these issues.

2 participants