Skip to content
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

Rework directives #663

Merged
merged 3 commits into from
May 31, 2022
Merged

Rework directives #663

merged 3 commits into from
May 31, 2022

Conversation

oryan-block
Copy link
Collaborator

@oryan-block oryan-block commented May 17, 2022

Checklist

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

Description

Following the addition of applied directives in v18 I wanted to have a permanent solution to how we handle directives.
This ended up being mostly a re-implementation of SchemaGeneratorDirectiveHelper so I'm not sure of the benefit.
The upside is that we no longer depend on copy-pasted code or deprecated API methods to wire the directives.


private fun <T : GraphQLDirectiveContainer> wireDirectives(wrapper: WiringWrapper<T>) {
val directivesContainer = wrapper.graphQlType.definition as DirectivesContainer<*>
val directives = buildDirectives(directivesContainer.directives, wrapper.directiveLocation)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Still waiting for an answer to my question here but building these directives on the fly seems to be the most likely solution, assuming that SchemaDirectiveWiringEnvironment#getDirective/s remains not deprecated and we have to supply them somehow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: this was fixed in graphql-java/graphql-java#2834
Still, we should only remove this once the deprecated methods are removed.

}

@Test
fun `should apply repeated directive`() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix for #615

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.

None yet

2 participants