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

Consider supporting promise-based dataloaders in v3 #1273

Closed
AlexCLeduc opened this issue Nov 25, 2021 · 1 comment
Closed

Consider supporting promise-based dataloaders in v3 #1273

AlexCLeduc opened this issue Nov 25, 2021 · 1 comment

Comments

@AlexCLeduc
Copy link

I know graphql-core dropped support for promises, but the author seems to think promise-support can be added via hooks like middleware and execution context (see response to my identical issue in graphql-python/graphql-core#148).

Since most people using syrusakbary's promise library are probably already graphene users, if anyone is going to help make graphql-core 3 and promises play together, it makes sense for that to be done in graphene.

Why not just use async?

I think I have a decent use-case for non-async, promise-based resolution. Async is nice and all, and having a standard is great, but many of us are just using dataloaders as an execution pattern, not because we actually have async data-sources. Moving everything to run in async environment can have consequences.

We are calling the django ORM from our dataloaders. Because django 3.0 forces us to isolate ORM calls and wrap them in sync_to_async, we stuck with promise based dataloaders for syntactic reasons. Examples below:

What we'd like to do, but django doesn't allow

class MyDataLoader(...):
    async def batch_load(self, ids):
        data_from_other_loader = await other_loader.load_many(ids)
      	data_from_orm = MyModel.objects.filter(id__in=ids) # error! can't call django ORM from async context. 
        # return processed combination of orm/loader data

What django would like us to do

class MyDataLoader(...):
    async def batch_load(self, ids):
        data_from_other_loader = await other_loader.load_many(ids)
        data_from_orm = await get_orm_data()
        # return processed combination of orm/loader data
    
@sync_to_async
def get_orm_data(ids):
    return MyModel.objects.filter(id__in=ids)

What we settled on instead (use generator-syntax around promises)

class MyDataLoader(...):
    def batch_load(self,ids):
        data_from_other_loader = yield other_loader.load_many(ids)
        data_from_orm = MyModel.objects.filter(id__in=ids)
        # return processed combination of orm/loader data

A simple generator_function_to_promise is used as part of dataloaders, as well as a middleware that converts generators returned from resolvers into promises. I have hundreds of dataloaders following this pattern. I don't want to be stuck isolating all the ORM calls as per django's recommendations. That would be noisy and decrease legibility.

It seems there may be other issues around using async dataloaders with connections. Admittedly that problem sounds more easily surmountable and wouldn't require supporting promises.

@AlexCLeduc
Copy link
Author

Oops, I meant to open this in graphene, not graphene-django

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant