-
Notifications
You must be signed in to change notification settings - Fork 96
Update to graphql-java 4.2 #105
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
| GraphQLObjectType object = GraphQLAnnotations.object(OptionalTest.class); | ||
| GraphQLSchema schema = newSchema().query(object).build(); | ||
|
|
||
| GraphQL graphQL = GraphQL.newGraphQL(schema).queryExecutionStrategy(new EnhancedExecutionStrategy()).build(); |
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.
Why did you remove the queryExecutionStrategy?
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.
He may removed it during development but forgot to put it back with its third commit.
Also https://github.com/graphql-java/graphql-java-annotations/pull/105/files#diff-9599d6f1ee8ac402b47f46e351eead38L644 and https://github.com/graphql-java/graphql-java-annotations/pull/105/files#diff-9599d6f1ee8ac402b47f46e351eead38L675
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.
Yes, I restored an EnhancedExecutionStrategy for gql4.2, but did not put it back on tests where it was not needed - it's only used for relay mutations, so kept it only in RelayTest. I could restore it in these tests also, but it won't change much.
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.
If its not necessary, no need
|
Looks like there are not many changes, it looks good. Can you write a little more detailed description about the parts you changed here? |
|
Here's the list of changes :
|
|
|
||
| public class EnhancedExecutionStrategy extends AsyncSerialExecutionStrategy { | ||
|
|
||
| private static final String CLIENT_MUTATION_ID = "clientMutationId"; |
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.
why do you have this const?
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.
Sorry I don't understand the question ? The const was not introduced in this PR but in f39d26f , and is still used in 3 places in the class.
|
Can you please resolve the conflicts? |
47532db to
b01a1ca
Compare
|
Is it ok to merge ? |
No major changes, however we could have a v3 branch to continue support on graphql 3.0