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

Add assumeValidSDL config option #3014

Closed
wants to merge 1 commit into from
Closed

Add assumeValidSDL config option #3014

wants to merge 1 commit into from

Conversation

jdolle
Copy link

@jdolle jdolle commented Nov 26, 2019

To allow more flexibility, I've exposed graphql's assumeValidSDL option. (ref: https://github.com/graphql/graphql-js/blob/master/src/utilities/buildASTSchema.d.ts#L45)

I am intentionally using multiple directives and have a custom schema compiler. There are PRs to allow multiple directives (sangria-graphql/sangria#409 graphql/graphql-js#1541), but adding this option allows for more possibilities and more flexibility for peoples' schemas.

@jdolle
Copy link
Author

jdolle commented Nov 26, 2019

Not sure why the check failed -- I ran test, build, build-website, and lint and they all passed.

@dotansimha
Copy link
Owner

dotansimha commented Nov 26, 2019

@jdolle It's on our internal CI, just ignore it :)

Thanks for the PR, I'll review it soon

@dotansimha
Copy link
Owner

Thanks @jdolle

I'm not sure that adding it in the root is the right choice, because we might want to allow this only for specific inputs.
What do you think?

@ardatan @kamilkisiela

@jdolle
Copy link
Author

jdolle commented Nov 27, 2019

Perhaps it would be better placed under the schema, similarly to the custom loader? I can make that change if so.

@ardatan
Copy link
Collaborator

ardatan commented Dec 8, 2019

@dotansimha This is something we should handle in graphql-toolkit's schema loaders. So we can move this to graphql-toolkit.

@dotansimha
Copy link
Owner

Sounds good, @jdolle , it can be under schema config.
@ardatan you are right, but we can merge it here and then later move it.

@ardatan
Copy link
Collaborator

ardatan commented Dec 23, 2019

#3147
This PR covers that functionality as an option to schema field like below;

schema:
  - your-schema-ptr:
      assumeValidSDL: true

@jdolle
Copy link
Author

jdolle commented Dec 27, 2019

Closing since #3147 covers this. Thanks

@jdolle jdolle closed this Dec 27, 2019
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

3 participants