-
Notifications
You must be signed in to change notification settings - Fork 50
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
Better async support #1
Comments
I see no issues with adding support for this, async support would be beneficial. |
I might be able to submit a PR for this with some pointing in the right direction. Let me know if you need help :) |
Thanks for the contribution, it needed a slight adjustment to the unit test to pass. Looks good! |
This was released to NuGet in version 0.2.1-beta: https://www.nuget.org/packages/Dapper.GraphQL/0.2.1-beta |
Interesting, it passed for me when I ran it alone. But I did get one failing when I "ran all" the existing tests together before adding the new test. Is this because this IDbConnection is being shared once per test run and a previous execution will close the connection so it needs to be re-opened? Otherwise I'd expect to see connection.Open() in all the resolvers before .Execute() or .ExecuteAsync() If so, should the lifetime of the IDbConnection in DI be changed? |
It's because the DbConnection that is used currently isn't thread-safe, so it's getting mangled by all the tests. Plus, the unit tests are terribly slow because of the LocalDB design, I'm going to try some options to speed it up and avoid these issues altogether. |
I see, that makes sense, thanks. I've been wondering myself about how best to manage the IDbConnection in my own asp.net core app. I see in the resolvers you're using the serviceProvider injected in the Query object, and so I should probably just configure a transient IDbConnection in my Startup like so:
|
Hey @dougrday, Do you think you could take a sec to release a new version with the rest of the async methods I added in there? Looking at using them now. Thanks mate! You can prob close this out then too. |
Remaining features were released to NuGet in version 0.2.2-beta: https://www.nuget.org/packages/Dapper.GraphQL/0.2.2-beta |
Thanks Doug! |
Setup a quick test project - this library works very well and is a real layer of sanity on top of graphql-dotnet. I noticed the querying is all sync, and presumably async would be better?
Looks like graphql-dotnet supports awaiting resolvers:
graphql-dotnet/graphql-dotnet#205 (comment)
At first glance, it looks like just
SqlQueryContext
needs anExecuteAsync
companion method.Would this make sense/be within the goals of your library?
The text was updated successfully, but these errors were encountered: