-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added DataFetcherFactory support #735
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
Added DataFetcherFactory support #735
Conversation
Adds a level of indirection into the data fetchers which are specified at schema def time but needed "wired" at query execution time
| * | ||
| * @return a data fetcher factory that always returns the provided data fetcher | ||
| */ | ||
| static <DF> DataFetcherFactory<DF> useDataFetcher(DataFetcher<DF> dataFetcher) { |
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.
not really sure about that one: Can we the static factory method to somewhere else?
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.
done
| public GraphQLFieldDefinition(String name, String description, GraphQLOutputType type, DataFetcherFactory dataFetcherFactory, List<GraphQLArgument> arguments, String deprecationReason, FieldDefinition definition) { | ||
| assertValidName(name); | ||
| assertNotNull(dataFetcher, "dataFetcher can't be null"); | ||
| assertNotNull(dataFetcherFactory, "dataFetcherFactory can't be null"); |
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.
this is confusing I think: until know we only have DataFetcher ... no we say DataFetcherFactory can't be null: maybe something like 'you have to provide a DataFetcher (or DataFetcherFactory)' ?
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.
done
| * | ||
| * @return a data fetcher | ||
| */ | ||
| DataFetcher<T> get(GraphQLFieldDefinition fieldDefinition); |
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 think it is safer for the future provide an ....Environment Object containing the fieldDefitinion, but also the parent "container".
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.
Great point - on it
…factory-support # Conflicts: # src/main/java/graphql/schema/GraphQLFieldDefinition.java
Adds a level of indirection into the data fetchers which are specified at
schema def time but needed "wired" at query execution time
see #734