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

Improve documentation on dataLoader using GraphQlBundle #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Improve documentation on dataLoader using GraphQlBundle #20

wants to merge 1 commit into from

Conversation

Ciloe
Copy link

@Ciloe Ciloe commented Mar 14, 2019

No description provided.

Signed-off-by: Jimmy ESCRICH <jimmy.escrich@medelse.com>
services:
graphql_promise_adapter:
class: Overblog\DataLoader\Promise\Adapter\Webonyx\GraphQL\SyncPromiseAdapter
public: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not have to be public.

class: Overblog\PromiseAdapter\Adapter\WebonyxGraphQLSyncPromiseAdapter
arguments:
- "@graphql_promise_adapter"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems very verbose, this was the only configuration that I needed:

    Overblog\DataLoader\Promise\Adapter\Webonyx\GraphQL\SyncPromiseAdapter: ~
    Overblog\PromiseAdapter\Adapter\WebonyxGraphQLSyncPromiseAdapter: ~
    Overblog\PromiseAdapter\PromiseAdapterInterface: '@Overblog\PromiseAdapter\Adapter\WebonyxGraphQLSyncPromiseAdapter'

Why not just register this out of the box?

function ($ids) use ($promiseAdapter) {
return $promiseAdapter->createAll($this->find($ids));
},
$promiseAdapter

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you are creating a callable? The batch_load_fn option is supposed to take care of this. Did this ever work? @mcg-web we're a bit lost I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my example here #22

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I went around the barn the long way, and concluded that while the original example isn't terrific, this new one is doing it wrong.

You are not supposed to inherit from Dataloader -- this bundle will create services which are instances of Dataloader, and those services will call the callable you define in "batch_load_fn".

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.

4 participants