-
Notifications
You must be signed in to change notification settings - Fork 1
Active record preload #40
base: master
Are you sure you want to change the base?
Conversation
ee3cc7f
to
56383de
Compare
Do we still need to keep https://github.com/GraphQLAcademy/swapi-graphql-ruby/blob/56383de8409ce7a52566cf4700e66f0992770d55/app/models/graph/association_loader.rb? There might be a case where we want to preload associations and |
@@ -23,8 +25,5 @@ module Graph | |||
model = Object.const_get(gid.model_name) | |||
Graph::FindLoader.for(model).load(gid.model_id.to_i) | |||
end | |||
|
|||
lazy_resolve(Promise, :sync) | |||
instrument(:query, GraphQL::Batch::Setup) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your gem includes the right setup? What happens if instrument(:query, GraphQL::Batch::Setup)
is included twice by accident? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does a NestedError
🤔. I've been trying to find a way to raise a useful error but since it's order dependant it's kinda hard :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if I could handle it better in the gem: use a Set instead of an Array to store instrumenters, that way a second call would do nothing.
resolve -> (person, _, _) do | ||
Graph::AssociationLoader.for(::Person, :homeworld).load(person) | ||
end | ||
preloads :homeworld |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat :)
Use https://github.com/xuorig/graphql-active_record_batcher to batch associations.
@cjoudrey