Re-use loaders after they have been performed, but clear on mutation #63
Conversation
|
This PR does not include all of the changes from the original #53. The missing changes don't seem particularly relevant, but I'm wondering either why they're not here or why you didn't strip them from that PR originally? |
|
I'm not 100% clear on why we need to wrap mutations in both the execution strategy and field instrumentation? It's been a while since I've dug into this gem, do we just have two entirely separate methods for integrating into the |
|
Just to be clear my understanding is correct, the batch loaders will now be cleared only:
|
| resolve ->(obj, args, ctx) { | ||
| GraphQL::Batch::Executor.current.clear | ||
| begin | ||
| Promise.sync(old_resolve_proc.call(obj, args, ctx)) |
There was a problem hiding this comment.
Is the sync necessary here? I didn't think it made sense for a mutation's resolve method to return a promise.
There was a problem hiding this comment.
If a loader were re-used in a mutation to load data to mutate, then it wouldn't be necessary to call sync on the promise. E.g. a callback chain like RecordLoader.for(Model).load(args[:id]).then { ... } could be used. That isn't a pattern that we use, but it doesn't make sense to prevent that considering graphql-ruby provides lazy loading support for mutation fields. So rather than change that behaviour, I just want to try to make sure the cache is actually cleared after any promise callbacks on a returned callback have been called.
|
A bunch of questions because it's been so long since I've looked at this code, but generally this makes sense. |
I added them back in the same PR that reverted them (#56) since they were in support of that change, but didn't cause the regression. |
Ya, I just hadn't ripped out the old execution strategy code yet. But that would only be needed to support a very old version of graphql-ruby. |
| if GraphQL::VERSION >= "1.6.0" | ||
| schema_defn.instrument(:multiplex, GraphQL::Batch::SetupMultiplex) | ||
| instrumentation = GraphQL::Batch::SetupMultiplex.new(schema) |
There was a problem hiding this comment.
What do you think of splitting the multiplex/query and field instrumenters in two different concerns? Setup does a lot right now I find.
There was a problem hiding this comment.
lib/graphql/batch/setup.rb is only 54 lines long, and only does two things which seem quite related. Also, when we want to break backwards compatibility, we can simplify the code by just having multiplex instrumentation and no need to share code between the two setup classes.
| old_resolve_proc = field.resolve_proc | ||
| field.redefine do | ||
| resolve ->(obj, args, ctx) { | ||
| GraphQL::Batch::Executor.current.clear |
There was a problem hiding this comment.
Not totally sure why you need to clear before and after?
There was a problem hiding this comment.
Clearing afterwards is to avoid using data cached from before the mutation. Clearing before the mutation isn't as important, but just ensures that the field is actually executed serially. However, I could see how it might be desirable to use the already loaded values in the mutation.
Do you think we should clear before the mutation? Or get rid of this one?
Fixes #43
Problem
#56 reverted a change that re-uses loaders after they are loaded so that they aren't re-used after a mutation.
Solution
Add field instrumentation to wrap the resolve proc for mutation fields in order to clear the loaders cache before and after it is called.
Identifying which fields were mutation fields required having the schema in the instrumentation, so I changed the setup modules into classes that take the schema as an initializer. Before #51 we recommended using
instrument(:query, GraphQL::Batch::Setup)in the README, so I have added class methods to deprecate that old usage.