Closed sudevva closed 1 week ago
Thanks for this @sudevva. I'll review soon. Anyone else that's using prefetch, a review would be appreciated.
@sudevva can I get a little more explanation on this PR? I'm having a hard time understanding why a boolean value is a good idea, and what returnRequested
even means.
👋 So about returnRequested. Basically previous when prefetch is used, the function with Prefetch annotation receives an array of resolved values, that should be mapped to source(like $prefetchResult[$blog->getId()]). This applies a limitation in a way, where you cannot use any Deffered/promise based code in prefetch method. When returnRequested is true:
About bool value for annotation, maybe it will be better to create another type of annotation and mark prevous one as depricated.
Previous implementation where method recieves an array of values creates some confusion for me, why I recieve an array from which I should extract my value.
Thank you @oojacoboo, I hope this will clarify my intentions)
Okay, so am I right in assuming that you added this boolean property as a way of achieving BC? Can we just merge the functionality without adding any additional properties to the Prefetch
attribute? It's confusing, I think. I don't see the purpose of it. Why can't the prefetch method return a promise without this boolean value? What advantage does this additional property, returnRequested
, provide?
Okay, so am I right in assuming that you added this boolean property as a way of achieving BC? Can we just merge the functionality without adding any additional properties to the
Prefetch
attribute? It's confusing, I think. I don't see the purpose of it. Why can't the prefetch method return a promise without this boolean value? What advantage does this additional property,returnRequested
, provide?
Do you think I should leave a prefetch behavior as it was previously? This is how I should map values each time Dataloader is used because I can return only one Promise from preload, that was my concern about default behavior
#[ExtendType(class: Video::class)]
class TagsExtension
{
#[Field]
/**
* @return VideoTag[]
* @param VideoTag[][] $videoTags
* @throws \Exception
*/
public function getTags(
Video $video,
#[Prefetch('preload')] array $videoTags
): iterable {
return $videoTags[$video->getId()]; // I can't return Promise here
}
/**
* @param Video[] $videos
* @return Promise<VideoTag[][]>
*/
public static function preload(iterable $videos, #[Autowire] VideoTagResolver $videoTagResolver): Promise {
return $videoTagResolver->getVideosTags(array_map(fn($video) => $video->getId(), $videos))
->then(function ($videoTags) use ($videos) {
$return = [];
foreach ($videos as $key => $video){
$return[$video->getId()] = $videoTags[$key] ?? [];
}
return $return;
}
);
}
}
Btw, I can wrap \TheCodingMachine\GraphQLite\QueryField::__construct $callResolver in deferred as I have done for args until return type is resolved to call assertReturnType with proper value, then I will not use Prefetch anymore. Take a look at https://github.com/thecodingmachine/graphqlite/pull/702/commits/ffbb68b75719b50c9c94a0e90f5d701233bc6ec6
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 95.18%. Comparing base (
53f9d49
) to head (ca0823a
). Report is 91 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
On https://github.com/thecodingmachine/graphqlite/commit/ffbb68b75719b50c9c94a0e90f5d701233bc6ec6, why is this being called unwrapReturnType
and not something more fitting like resolveWithPromise
?
The overall goal would be to create a more uniform implementation for resolving. Promises seem like a good idea to me for preload functionality. Wrapping non promise prefetch returns seems like a logical way of implementing things.
Please be sure to put line spaces between your control structures - easier for people to read. We're not worried about trying to optimize file sizes.
hey, I renamed a method, added a forgotten nesting levels test, and added some line spaces.
Is this PR more or less ok?
If you want I can squash and rebase commits. There is no value in commits about CR fixes and reverting my initial changes.
No that's fine. We do need the docs updated though, with details on using promises. That's the website/docs
dir.
Promise mapping added
Thanks @sudevva!
Fix the bug with prefetching on different nesting levels As a side effect, it's now allowed a Promise to be returned from prefetchMethod with returnRequested: true, so Overblog Dataloader can be used. Also, there's no need to map a prefetched result to return a value.
update version of related PR #519 also can potentially help with #187