-
Notifications
You must be signed in to change notification settings - Fork 85
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
Improve introspection support #83
base: master
Are you sure you want to change the base?
Conversation
…flect changes in unit test. chkimes#47
…elated to type conditions in queries. chkimes#47
Auto commit by GitBook Editor
Auto commit by GitBook Editor
Auto commit by GitBook Editor
Auto commit by GitBook Editor
Auto commit by GitBook Editor
Auto commit by GitBook Editor
Auto commit by GitBook Editor
…ql-net into introspection
Auto commit by GitBook Editor
@ckimes89 will you find time to look at this PR or shall i merge it? |
@MarianPalkus Hey, sorry for the delay - I've been pretty busy for a while. I'll try to make some time to review this during this week. If I haven't left any comments by the weekend, feel free to merge it. Same goes for the other PR. |
@ckimes89 No problem, thanks for your response. |
@MarianPalkus Thanks for all your hard work on this! I think I'm missing some context around the |
Thanks for reviewing and sharing your thoughts. The For fields with an union types of The idea of I had no better idea to solve this problem yet. |
Ah, I see. So for situations like https://github.com/ckimes89/graphql-net/pull/83/files#diff-45f447cd065642a16ffbb95b53ab29acR118 where you don't have a CLR type that represents our GraphQL type. I'll think about this one for a bit. I think both the current solution and a new overload with the return type parameter specified are less than ideal since 1) you don't get the benefits of type inference or checking and 2) the results of the schema-building API are now potentially used as inputs to the same API. To clarify on 2, previously it was CLR types and expressions going in, and GraphQL types/fields coming out. This isn't a horrible thing and I'd be willing to accept it given no other options, but it just makes the API a bit more convoluted. One option to consider would be a special union CLR type, like
Admittedly it's not the prettiest creation, but that could be somewhat alleviated with a type alias like |
I agree, that is a much nicer API. I try to change it this way. |
I'm also open to any other suggestions. CLR Union types are a big addition, so I want to make sure it's worth it. Also, I think something like Entity Framework could have problems trying to deserialize into a Union type (which it might try to do since it's part of the expression). There are a number of questions to be answered here. |
Additionally, imho the spec does allow defining two different unions with the same set of object types. |
The goal of this PR is to improve support of introspection which is needed to use tools (e.g. GraphiQL, apollo-codegen, ...).
Breaking Changes:
Abstract types (interface and union types) can be defined using:
GraphQLSchema.AddUnionType(string name, IEnumerable<IGraphQLType> possibleTypes, Type type = null string description = null)
GraphQLSchema.AddInterfaceType<TInterface>(string name = null, string description = null)
Interface implementations can be defined using:
GraphQLTypeBuilder.AddInterface<TInterface>(GraphQLTypeBuilder<TContext, TInterface> interfaceTypeBuilder)
Example:
Since those changes are quite extensive, it adds more unit tests (and fixes of minor occurring issues) which makes this PR is so large.
Changes:
Related to #8, #47, #80.
TODOS:
MutationType
is missing"System.Data.Entity.DynamicProxies
in mutation results (GraphQL.Net/Executor.cs:28
): Should this be anExpressionOption
? Separate PR Add expression option to use base type. #85GraphQL.Parser.Test.SchemaTest.TestRecursionDepth
fails. Any help is very welcome.This PR is still work-in-progress - reviews, tests/hints related to edge cases and other remarks are very welcome.