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

Add data loader overloads to MicrosoftDI resolver builders #3868

Merged
merged 6 commits into from
Jan 3, 2024

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Jan 1, 2024

@Shane32 Shane32 added the data loader Relates to GraphQL data loaders label Jan 1, 2024
@Shane32 Shane32 added this to the 7.8 milestone Jan 1, 2024
@Shane32 Shane32 self-assigned this Jan 1, 2024
@Shane32
Copy link
Member Author

Shane32 commented Jan 1, 2024

Questions:

  • Should this extension method exist only in the DataLoader library? Or only the MicrosoftDI library? Or perhaps the resolver builder feature of the MicrosoftDI library should have additional overloads for ResolveAsync that accept data loaders, similar to the added overloads to ResolveAsync ?

@Shane32 Shane32 added the enhancement New feature or request label Jan 1, 2024
@Shane32 Shane32 requested a review from gao-artur January 2, 2024 06:37
@gao-artur
Copy link
Contributor

I think adding overrides to MicrosoftDI is cleaner. The ReturnsDataLoader is another level of complexity that you must know to define the field. If I were going to convert my existing code to use ResolverBuilder, I'd expect only to add .Resolve().WithService<T>(). But instead, I need to start looking around to find out I also need to add ReturnsDataLoader. This will also complicate implementation of this analyzer #3781

@Shane32 Shane32 changed the title Add ReturnsDataLoader method Add data loader overloads to MicrosoftDI resolver builders Jan 2, 2024
@Shane32
Copy link
Member Author

Shane32 commented Jan 2, 2024

I think adding overrides to MicrosoftDI is cleaner. The ReturnsDataLoader is another level of complexity that you must know to define the field. If I were going to convert my existing code to use ResolverBuilder, I'd expect only to add .Resolve().WithService<T>(). But instead, I need to start looking around to find out I also need to add ReturnsDataLoader. This will also complicate implementation of this analyzer #3781

Done

@codecov-commenter
Copy link

Codecov Report

Attention: 73 lines in your changes are missing coverage. Please review.

Comparison is base (911afe8) 84.66% compared to head (2fb2570) 84.35%.
Report is 2 commits behind head on master.

Files Patch % Lines
src/GraphQL.MicrosoftDI/ResolverBuilder.cs 6.41% 73 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3868      +/-   ##
==========================================
- Coverage   84.66%   84.35%   -0.32%     
==========================================
  Files         419      420       +1     
  Lines       19333    19439     +106     
  Branches     3033     3037       +4     
==========================================
+ Hits        16369    16397      +28     
- Misses       2251     2328      +77     
- Partials      713      714       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Shane32 Shane32 merged commit 9f46f9c into master Jan 3, 2024
11 checks passed
@Shane32 Shane32 deleted the returns_dataloader_method branch January 3, 2024 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data loader Relates to GraphQL data loaders enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot implicitly convert type IDataLoaderResult<T> to T
3 participants