-
Notifications
You must be signed in to change notification settings - Fork 285
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
Automatic data loader naming on classes annotated with nameless @DgsDataLoader #904
Automatic data loader naming on classes annotated with nameless @DgsDataLoader #904
Conversation
Thanks for the PR. The changes look good to me, I need to test it out a bit before merging. Will do that this week. |
Please look at the automatic field naming the closest, I am seriously debating leaving it out (and throwing a exception on unnamed field dataloaders) because refactoring the field name breaks code (fields cannot be referenced as parameters in methods |
graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/DgsDataLoaderProvider.kt
Outdated
Show resolved
Hide resolved
Regarding #904 (comment) I agree, I think generating the names automatically for the field loaders feels like it may result in more confusion than help. Let's remove that and keep just the generation of class data loader names. Otherwise looks good to me. Thanks for the PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can merge the PR after you get a chance to remove the generation of names for field data loaders. Thanks!
graphql-dgs/src/main/kotlin/com/netflix/graphql/dgs/internal/utils/DataLoaderNameUtil.kt
Outdated
Show resolved
Hide resolved
Made the following changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. Will merge the Pr.
Pull request checklist
first
Pull Request type
Changes in this PR
This PR automatically names dataloaders, which is useful when using the
DgsDataFetchingEnvironment#getDataLoader(Class)
method.It also implements automatic naming of field loaders, which is arguably less useful. If anyone objects to this we can error out on nameless field dataloaders.Something to consider is that metric and tracing solutions may expose (field?)/class/package names of the dataloader. In the future we can add a
DgsDataLoaderNamingFactory
or something that gives library consumers the ability to modify the name programmatically (e.g. - only use the class name instead of the FQN) - this PR does not implement this.Partially related to #861
Alternatives considered
Name the loaders manually (gets old quickly)