-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Merge lazy resolution into Dataloader #5422
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
base: master
Are you sure you want to change the base?
Conversation
That’s interesting… so I could just go through and queue a whole bunch of evaluate_selections across fields and scopes, then just run dataloader once at the end of everything and then go through collecting results? Sound like a nice simplification! |
… in FlatDataloader; eagerly resolve mutation fields
…ation; fix backtrace_spec
@@ -389,11 +413,11 @@ def execute_multiplex(multiplex:) | |||
# All of these get `3` as lazy value. They're resolved together, | |||
# since they aren't _root_ mutation fields. | |||
"lazyValue" => 3, | |||
"i2" => { "value" => 2, "lazyValue" => 3 }, |
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.
This is actually a pretty big change. Even without use GraphQL::Dataloader
, fields may be run non-depth-first. In this case, i2
ran, then i3
, then i2.value
(which now gets 3
instead of 2
), then i3.value
. I added incrementedValue
to demonstrate that the fields do run as expected, but child fields run out of order.
If this test had already used GraphQL::Dataloader, it would not have passed as written. But the change from NullDataloader to FlatDataloader required this change.
Extracted from #5389
Unify resolution of Lazy objects (Promises) with Dataloader.
TODO:
Resolve
back to the project with deprecation warningssteps_to_rerun_after_lazy
if it's unusedcc @gmac perhaps relevant to your interests, this change would make
dataloader.run
do everything