-
Notifications
You must be signed in to change notification settings - Fork 1.2k
This makes sure every introspection type actually has a concrete data fetcher #3004
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
Conversation
… fetcher behind it
| register(__InputValue, "defaultValue", defaultValueDataFetcher); | ||
| register(__InputValue, "isDeprecated", isDeprecatedDataFetcher); | ||
| register(__InputValue, "deprecationReason", deprecationReasonDataFetcher); | ||
| } |
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.
I reorganised the registration to that the register call is done in simple lines and the fetchers are defined above.
I also ordered them in the order of field definition from above.
So you can see that we defined name field first, so its DF registration is first and so on.
| .deprecate("see `specifiedByURL`") | ||
| .name("specifiedByUrl") | ||
| .type(GraphQLString) | ||
| .deprecate("This legacy name has been replaced by `specifiedByURL`") |
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.
Tweaked the comment while I was here - boy scout!
| .field(newFieldDefinition() | ||
| .name("onField") | ||
| .type(GraphQLBoolean) | ||
| .deprecate("Use `locations`.")) |
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.
onOperation / onFragment and onField was removed from the spec in 2016 - this has just sat around. I think it was broken. We have NO data fetcher defined AND there is not getOnFragment method that would give a value so it would always be null
I took the opportunity to remove this very old code
| import graphql.Assert; | ||
| import graphql.GraphQLContext; | ||
| import graphql.Internal; | ||
| import graphql.PublicApi; |
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 tests pick up the fields in place - we have great coverage here.
I put a debug point in PropertyDataFetcher when running the main tests and I never hit it for introspection related calls - so one more reason on why its not all covered by actual DFs
For runtimes like GraalVm we dont want to introduce
PropertyDataFetcherreflection to find the system__Typestyle objects which are really based of thegraphql.schema.xxxobjectsThis adds explcit data fetchers for the small amount of fields that ended up using
PropertyDataFetcher