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

why do loaders need to be in context? #240

Open
Tsugami opened this issue Sep 21, 2021 · 5 comments · May be fixed by #247
Open

why do loaders need to be in context? #240

Tsugami opened this issue Sep 21, 2021 · 5 comments · May be fixed by #247

Comments

@Tsugami
Copy link

Tsugami commented Sep 21, 2021

this is not such an obvious thing when trying to use the lib, and it becomes difficult to use it

suggestion:

  1. use the direct loader without having to add it in the context
  2. create a required parameter in createLoader to show where to consume the loader
@daniloab
Copy link

daniloab commented Sep 21, 2021

can you give examples of your idea?

@sibelius
Copy link
Member

We need loaders in GraphQL context because they use dataloader under the hood to provide cache and batch for our database queries

you should not share this cache between requests

what do you recommend?

@Tsugami
Copy link
Author

Tsugami commented Sep 21, 2021

Suggestion 1,

https://github.com/entria/graphql-mongo-helpers/blob/master/src/createLoader.ts#L80
context.dataloaders[loaderName]
here it's get from the context, instead it's get direct of createLoader function internally

Suggestion 2,

createLoader({
  name: 'UserLoader',
  getLoader: (ctx) => ctx.userLoader; // here it is optional for the developer to choose where to get it.
})

on second thought, maybe the first suggestion is not so valid, because it creates an instance globally and not for request

@Tsugami
Copy link
Author

Tsugami commented Sep 21, 2021

i didn't understand the need of context loaders, maybe how it's currently is the best way

@sibelius
Copy link
Member

I do like your second suggestion, feel free to implement it

add tests to avoid regressions, and keep the old default behavior

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 a pull request may close this issue.

3 participants