Skip to content

Conversation

@sergehuber
Copy link
Contributor

@sergehuber sergehuber commented Apr 20, 2017

Hello,
I'd like to propose the following changes:

  • Renaming the root query and mutation types from "query" and "mutation" to "Query" and "Mutation" to follow object type naming conventions
  • Modified the GraphQLQueryProvider implementation to make the old API deprecated and replace with a collection of field definitions, making this aligned with the GraphQLMutationProvider interface. This makes it possible to define data fetcher for the root object type fields.
  • Added JavaDoc documentation to the GraphQLQueryProvider interface

I think these changes would make the exposed GraphQL API a lot cleaner and also a lot more flexible. The inability to use data fetchers on the fields or list types in the root object types is a very strong limitation.

If these changes are approved I'd be willing to update the README to give some more documentation on how to use the new API.

In order to keep the API compatible, all the methods in the GraphQLQueryProvider are now optional, but over time we should change this to remove the methods now marked as deprecated.

The name change for the root object types should not have major impacts, these are just types names but they should be noted as well. Btw I noticed that the SimpleGraphQLServlet also uses a capitalized "Mutation" type so this is consistent with these changes.

- Renaming the root query and mutation types from "query" and "mutation" to "Query" and "Mutation" to follow object type naming conventions
- Modified the GraphQLQueryProvider implementation to make the old API deprecated and replace with a collection of field definitions, making this aligned with the GraphQLMutationProvider interface. This makes it possible to define data fetcher for the root object type fields.
- Added JavaDoc documentation to the GraphQLQueryProvider interface
@sergehuber
Copy link
Contributor Author

@apottere
Copy link
Contributor

I'm all for making the query provider return a list of fields instead of objects - this was one of the most glaring usability issues with it. I wouldn't worry about deprecating anything, this project is so young we can just make some breaking changes and release it as a new major version.

* @deprecated use query field definitions instead
* @return a GraphQL object type to add to the root query type
*/
default GraphQLObjectType getQuery() { return null; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my comment, I don't think there's much need to preserve this as deprecated right now. Once this is removed I'll merge this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll make the changes then, I'll remove all the deprecated methods. I'll let you know when I've pushed the changes.

- Modify tests to use new field definition method
@sergehuber
Copy link
Contributor Author

Ok I've pushed a new version that removes the old methods, and I've also modified the automated tests to make sure they use the new API. Travis build has executed successfully. Let me know if you want any more changes.

@sergehuber
Copy link
Contributor Author

sergehuber commented Apr 20, 2017

Ok I've done one last change, renamed the method from getQueryFieldDefinitions to getQueries so that it's consistent with the getMutations method of the Mutation provider. I'm not too convinced about this naming scheme but I haven't found a better one :)

@apottere
Copy link
Contributor

Awesome, thanks!

@apottere apottere merged commit a98b58a into graphql-java-kickstart:master Apr 20, 2017
@apottere
Copy link
Contributor

This is released in 3.0.0.

@sergehuber
Copy link
Contributor Author

Thanks Andrew for doing this so fast !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants