Skip to content

Conversation

@lburja
Copy link
Contributor

@lburja lburja commented Dec 2, 2020

Resolves #457

Checklist

  • Pull requests follows the contribution guide
  • New or modified functionality is covered by tests

Description

Upgrade graphql-java library to 16.1, and fix issues
All tests pass, and the library seems to work fine on some internal projects that I have

@lburja
Copy link
Contributor Author

lburja commented Dec 5, 2020

@vojtapol or @oliemansm, any feedback on this pull request? Is it acceptable for migration to graphql-jave 16.1?

@vojtapol
Copy link
Member

vojtapol commented Dec 8, 2020

@lburja We want to make one last release on graphql-java 15.0 with all the recent bug fixes for people that are not ready to upgrade to graphql-java 16.0. Then we will merge this.

@vojtapol vojtapol mentioned this pull request Dec 8, 2020
2 tasks
@vojtapol vojtapol added this to the 11.0.0 milestone Dec 8, 2020
@lburja
Copy link
Contributor Author

lburja commented Dec 8, 2020 via email

@vojtapol vojtapol requested a review from oryan-block December 23, 2020 16:17
Copy link
Collaborator

@oryan-block oryan-block left a comment

Choose a reason for hiding this comment

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

Looks good overall.

Some noteworthy changes from that releaase:

  • Use of immutable collections may break some things when upgrading but should be easy to fix.
  • We'll probably need to do some work to support repeatable directives as well.
// directiveDefOpt = FpKit.findOne ...)
// Note 2: repeatable directives (new feature in graphql-java 16) likely don't work,
// since we don't pass in the full set of discovered directives
return super.buildDirective(null, directive, setOf(graphQLDirective), directiveLocation, comparatorRegistry)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just going to note that this was made package protected in this PR: graphql-java/graphql-java#2058
I don't like this solution but it will do for now.
We may want to consider reflection for a more permanent solution.

@vojtapol vojtapol merged commit 5cd97c5 into graphql-java-kickstart:master Jan 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants