Field Arguments #56
Field Arguments #56
Conversation
Appears to be some quirk with Traits perhaps, as this should implicitly upcast into an Object but it fails within this trait. This works everywhere else the ExecutesClosures trait is used.
Lets us be a bit more DRY as we reuse this
With the exception of PersistentGraphQLProperties
| .deprecate(prop.deprecationReason) | ||
| .description(prop.description) | ||
|
|
||
| if(prop instanceof CustomGraphQLProperty) { |
There was a problem hiding this comment.
This felt a little gross to me, but no other place stood out as a better alternative for actually building the property-level arguments.
There was a problem hiding this comment.
Agreed. You should use something like:
if(prop instanceof Arguable) {
List<GraphQLArgument> arguments = prop.getArguments(typeManager, mapping)
if (!arguments.isEmpty()) {
field.arguments(arguments)
}
}
core/src/main/groovy/org/grails/gorm/graphql/entity/operations/CustomOperation.groovy
Show resolved
Hide resolved
| * @return The operation in order to chain method calls | ||
| */ | ||
| T argument(String name, Class type, @DelegatesTo(value = SimpleArgument, strategy = Closure.DELEGATE_ONLY) Closure closure = null) { | ||
| T argument(String name, Class<?> type, @DelegatesTo(value = SimpleArgument, strategy = Closure.DELEGATE_ONLY) Closure closure = null) { |
There was a problem hiding this comment.
Similar, in that when the implementing GORM entity used @GrailsCompileStatic, this would result in a static compilation error without the generic wildcard.
jameskleeh
left a comment
jameskleeh
left a comment
There was a problem hiding this comment.
Overall this PR is pretty good. I think if we're going to introduce breaking changes by moving the argument classes, and modifying this method signature (https://github.com/grails/gorm-graphql/pull/56/files#diff-65191fd62016a5999609edc95174e48c4937c21dfe6bae9773874e3efbea5169R55), we should also do the upgrades to Groovy 3, etc and include it in Grails 4.1
Let me know if you would prefer to proceed without breaking changes and include this in an earlier release
| .deprecate(prop.deprecationReason) | ||
| .description(prop.description) | ||
|
|
||
| if(prop instanceof CustomGraphQLProperty) { |
There was a problem hiding this comment.
Agreed. You should use something like:
if(prop instanceof Arguable) {
List<GraphQLArgument> arguments = prop.getArguments(typeManager, mapping)
if (!arguments.isEmpty()) {
field.arguments(arguments)
}
}
|
I'm fine with including it in Grails 4.1, although I'm a little unclear how it's a breaking change. None of the original functionality of the DSL is altered from what I can see. I'll go ahead and write up some doc changes to go with it. |
- Pushed them up a level to help imply they can be used for both operations and fields - Added a bit to the examples to emphasize usage
|
@jameskleeh can you clarify what you meant by breaking changes? From what I can tell, nothing exposed in the public API/DSL has been altered, only additions. For the sake of just taking baby steps, I'd rather avoid doing the upgrade to Groovy 3/Grails 4.1 in this PR. I'd be happy to do so in another though. |
|
@jamesdh This is the breaking change https://github.com/grails/gorm-graphql/pull/56/files#diff-65191fd62016a5999609edc95174e48c4937c21dfe6bae9773874e3efbea5169R57. Someone could be extending this class |
And build field arguments as seperate method to avoid any possible breakages
|
@jameskleeh this should be good now. Feel free to take another look! |
|
@jeffbrown @jameskleeh @puneetbehl I understand you're all fairly focused on Micronaut these days and that's completely fair. But would it be possible to promote some additional people to this repo to move things along? There are lots of things that could be improved and I'd be happy to contribute some of my own time to do so, but at this point it's starting to feel all for naught. |
|
@jamesdh Reviewing these PRs and creating a release is on my list of things to do. I assure you it is not all for naught |
core/src/main/groovy/org/grails/gorm/graphql/types/output/AbstractObjectTypeBuilder.groovy
Outdated
Show resolved
Hide resolved
|
Thanks for the update and the review @jameskleeh! |
2 participants
Fixes #53. Sorry for taking so long to submit this!
A core part of the GraphQL spec is the ability for fields to take arguments. This PR adds the ability to define custom fields on an object which in turn take arguments. It does not contain the ability to define arguments on existing fields.
I may have been overzealous in upgrading some of the gradle bits. Feel free to let me know if you'd like me to resubmit without those changes.
This PR does not yet contain any related documentation changes, hence the draft status. Wanted to get a little feedback before putting any time into them.