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

How to pass a single DataLoader instance to all graphql request handlers? #9

Open
ryancole opened this issue Jul 22, 2016 · 18 comments

Comments

@ryancole
Copy link

I'm trying to implement DataLoader which batches together underlying database queries within a single request. I'm looking towards react-relay-network-layer to assist with the batching of GraphQL queries into a single HTTP request so that I can use a single DataLoader for all queries within that request.

I notice that graphqlBatchHTTPWrapper calls the graphqlHTTPMiddleware for each query. I need to make accessible to each graphqlHTTPMiddleware call the same instance of my loaders. How do you suggest that I go about this?

Prior to adding react-relay-network-layer, or any batching, just using the default relay network layer, I was simply instantiating my DataLoaders right within the express-graphql middleware function and passing them to the resolvers via rootValue. I can no longer do that there because that'll create new DataLoaders for each query. I need a single DataLoader per request.

Looking for your advice. Thanks!

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

Please provide your prior realization with express-graphql. It helps me point you to right existed solution, or create new one based on your example.

@ryancole
Copy link
Author

Sorry about how possibly verbose this explanation is, but I'm just trying to provide enough to show you my situation.

Here is a link to where I use the react-relay-network-layer GraphQL-related code, so that I can support batched queries (in a single HTTP request) on the GraphQL side of things ...

https://github.com/ryancole/league/blob/dataloader/src/graphql/server.js#L22-L30

And here is the express-graphql middleware handler in which you would normally handle a single HTTP request to the default graphql-express handler, but in my code I've wrapped using react-relay-network-layer ...

https://github.com/ryancole/league/blob/dataloader/src/graphql/util/handleGraphqlRequest.js#L7

That code above shows where I create my DataLoader instances.

Now, my intention is for these DataLoader instances to be made available to express-graphql via rootValue but in such a way that one instance of them is used per request. The code that I linked does not work, because the express-graphql middleware function is executed per query, as opposed to per request, when using react-relay-network-layer.

@ryancole
Copy link
Author

Just another reply trying to explain my issue.

I'm needing to be able to provide rootValue but a single instance of it to all express-graphql handlers. I essentially need to create a variable, pass it through react-relay-network-layer, and into each request handler.

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

Awesome explanation, straight what I wanted!

Right now does not exists solution for your case. But it's not hard for implementation.
I now prepare proposal for review.

@ryancole
Copy link
Author

ryancole commented Jul 22, 2016

I think I could just make an express middleware that pins my DataLoaders to the request object, before the react-relay-network-layer code runs, which would make them available to my GraphQL middleware down the line. That wouldn't require any changes to RRNL.

Edit: Although I'm not sure how logical that is, considering the loaders have nothing to do with the request itself and probably don't need to logically be associated with the request object.

@ryancole
Copy link
Author

Alright well, I went ahead and made this express middleware that creates my DataLoader instances, once per request, and pins them to the request object.

https://github.com/ryancole/league/blob/dataloader/src/graphql/util/expressDataLoaderMiddleware.js

I'm not sure that it is technically logical, but it gets me moving forward. If you think that you've got a better idea about how to possibly pass parameters through react-relay-network-layer's express middleware (possibly HOF) then I'll just keep checking back! Thanks!

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

Finally, I found the same solution as you provide, based on your first detailed explanation.

I just comitted example: https://github.com/nodkz/react-relay-network-layer/blob/master/examples/dataLoaderPerBatchRequest.js

Please review it and leave a message, if found some errors.
And many thanks for your issue, help and examples. I'll use same code in my project.

If no any additions, you may close issue.

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

Will be great if you fix typos and misunderstanding places in comments, right via github web-editor https://github.com/nodkz/react-relay-network-layer/edit/master/examples/dataLoaderPerBatchRequest.js

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

And last question, why do you pass DataLoader to rootValue instead of context?
I think that context is the better place for such per request things by graphql design.

@ryancole
Copy link
Author

ryancole commented Jul 22, 2016

@nodkz Regarding your question about rootValue versus context. Based on my understanding, context is for session based data, such as current user, etc, while rootValue is for "everything else." The spec leaves this pretty open ended, and I made my decision based on some github comments by graphql contributors. I don't have links handy, but that's my recollection. I consider these data fetching functions to not be session related and more categorized as "everything else." :)

I'll go over your adjustments in just a few minutes.

@taion
Copy link
Member

taion commented Jul 22, 2016

BTW, compare my PR adding this functionality to express-graphql.

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

@ryancole thanks for answer! Really this is a tricky question. I'm confusing, but till not change my decision due context word meaning 😜 And all that based on user request, should be in context, even fetchers.

Asked Lee for advice with context and rootValue https://twitter.com/nodkz/status/756378662711267328

@ryancole
Copy link
Author

ryancole commented Jul 22, 2016

@nodkz true. I personally don't see the clear difference between context and rootValue. That was a somewhat ambiguous change to GraphQL, imo.

@taion I'll check it out, thanks. @nodkz, I do believe this is the PR that taion is talking about: graphql/express-graphql#100

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

DataLoaders should be in context

screen shot 2016-07-23 at 0 13 09

@taion
Copy link
Member

taion commented Jul 22, 2016

IMO you should really share both rootValue and context across all queries in a batch... I guess depending on the semantics you want. Like really a "batched query" is just a single query that happens to have multiple query roots, with some workarounds for various Relay compat reasons.

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

There are no restrictions in sharing: https://github.com/nodkz/react-relay-network-layer/edit/master/examples/dataLoaderPerBatchRequest.js - one config for all queries per one http request.

We just try choose better place for DL.

@taion
Copy link
Member

taion commented Jul 22, 2016

No, I mean that there's not really much reason to not always share context and rootValue across all GraphQL requests in a batched HTTP request.

@nodkz
Copy link
Collaborator

nodkz commented Jul 22, 2016

Ah, completely agreed with you.

Too scary and complex config for readme.md. I wanted write something hacky again. But I'll prefer wait your PR: graphql/express-graphql#100 And remove tons of my rubbish code.

Will pray 😉 that @leebyron accept it.

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

3 participants