-
Notifications
You must be signed in to change notification settings - Fork 79
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
Architecture using timeouts to call fetch? #14
Comments
IIRC the facebook implementation batches together everything on the same "tick", golang doesn't have ticks. The timeout gives you the freedom to tweak how long you want to wait for a batch. Now we could kind of simulate a tick in gqlgen right after starting all the concurrent resolvers (and maybe a gosched to make sure they have a chance to run). Given dataloaders are usually in context, this probably needs some smart context magic, and ideally can be done in a way that doesn't tie gqlgen directly to dataloaden and allows other libraries to receive that "resolvers issued" tick, if they want it. In practice short timeouts (~1ms) work well enough and there are bigger fish to fry, but if someone wants to spec it out properly, and open to a PR that would be awesome :) |
related 99designs/gqlgen#518 (comment) |
I've so far been using graphql in node and have used the facebook dataloader quite a bit. I have some go services that I want to add a graphql api to. Gqlgen seems quite legit and I've read good things about it so far. However the way this dataloader works doesn't feel right. The facebook dataloader in js doesn't have such a weird implementation where you have to wait an arbitrary amount of time to resolve the data. Did you have trouble finding a solution in go that doesn't involve timeouts to know when to resolve the data? I would like to explore this a bit as it's currently preventing me from deciding to use this library. Can you maybe give some info on what you tried and how you decided that timeouts is the best solution? I think it would be good to include this in the readme, as I'm sure I'm not the only one that has concerns about this.
The text was updated successfully, but these errors were encountered: