-
Notifications
You must be signed in to change notification settings - Fork 54
Interface and union resolvers #149
Interface and union resolvers #149
Conversation
packages/graphqlgen/src/tests/typescript/__snapshots__/basic.test.ts.snap
Outdated
Show resolved
Hide resolved
packages/graphqlgen/src/tests/typescript/__snapshots__/basic.test.ts.snap
Outdated
Show resolved
Hide resolved
Thanks a lot for this PR. Would you mind testing it locally, so we can merge this PR with confidence? 💪
Yes, we're in the process of refactoring the tests as well. See #157. |
Just updated some minor things, should be good now. As in, works on my machine. |
For even better support I could also generate an interface type, which implements the interface's resolver fields, and can be used as a parent type to prevent writing resolvers for implementing types twice. |
Thanks a lot @koenpunt. I've just taken a look. A few things:
|
@schickling Regarding the scaffolding behavior; I'll be on a plane from Amsterdam to Las Vegas, and I expect that to be done when I land in about 14 hours. |
Is my understanding correct that you either have to implement
What are your thoughts? I'm kind of leaning to (2) for now and eventually (3). Have a safe flight 🛬 |
@schickling alright, good to know. Yeah I'm using the library already myself, so I kinda need it, but I have to rebase again etc. |
I'm not going to work on this before #244 is merged, because I have a feeling I have to start over again once that lands... |
#244 is now merged. Do you want to tackle this again? (Sorry for the overhead but I think the refactor was well worth it) 🙏 |
I will have a go at it tonight or tomorrow night |
Typescript now works. For flow there are some types not being generated yet. |
Is this close? This would greatly benefit a project that I am working on. |
I'd like to get some input on what I created thus far. @schickling you have any remarks? Or are you totally fine with where this is going and are you just waiting for me to finish the flow implementation too? |
@schickling do you have any feedback? |
@schickling This PR seems to be ready to merge. Is there anything missing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks great @koenpunt thank-you!
packages/graphqlgen/src/tests/typescript/__snapshots__/basic.test.ts.snap
Outdated
Show resolved
Hide resolved
@schickling I think there could be a fourth option actually, by leveraging a union to force developers into one of many valid implementations. Its like raising both options in your config option to the type level : ) That said, right now, I don't think the added implementation complexity is worth it; and the potential implications for type comprehensibility would need more consideration. @anotherhale Yes I think its close. I will be checking in with @timsuchanek about it this week. |
packages/graphqlgen/src/tests/typescript/__snapshots__/large-schema.test.ts.snap
Outdated
Show resolved
Hide resolved
I'm not 100% convinced but all things considered I think overall a good next step is to merge this and make a 0.6.0-rc1 release. There's a lot of refactoring I want to do, and from reading the comments flow type may not be 100% complete, but I think shipping something for TypeScript users is worth it. The release notes will be clear that flowtype support is not guaranteed. |
Hi @jasonkuhrt I'm sooo happy that this is being implemented! Gave me a headache all day before I found the PR. Just discovered an issue though: The generated resolvers' index.ts doesn't seem to include the Interface resolvers. Since the resolvers are all optional you won't discover this when copying the scaffolding, only after executing them. I guess this is a bug? |
@maggo cheers : ) Bug or feature, either way, feel free to create an issue, supply some context/example and we can discuss further there 👍 Thanks! |
@jasonkuhrt Nice to see this is merged, since this means I soon no longer have to run my own fork to use Also, your modifications caused a generation issue, that wasn't there before: args: {},
ctx: Context,
info: GraphQLResolveInfo
- ) => Channel | null | Promise<Channel | null>;
+ ) =>
+ | Channel
+ | Channel
+ | Channel
+ | null
+ | null
+ | Promise<Channel | Channel | Channel | null | null>; And lastly I'm wondering why you removed the |
Sorry to hear that, it was actually to give you credit on master branch. It felt to me the least I could to thank you for the contribution. However I forgot or neglected to post a notice about continuing work on this branch (whereas I did do that on some other PRs). So, sorry that you were taken by surprise 🙏. I don't think I was seriously expecting a reply from you so that probably contributed to lowering my communication standards. Thanks for sharing your feeling on this.
We're currently in an rc stage. If you could create a new issue with context that would be helpful toward getting a fix out.
Could you elaborate? Ideally in a new issue, thanks! |
This is a new version of #66.
I didn't test the generated files yet, but the test snapshot looks promising.
It generates a resolver for the interface and union types in the following shape;
And it generates a
__isTypeOf
resolver on types that implement an interface or are part of a union.Also, what's up with all the
.tmp
tests? Barely anything is covered with tests at the moment..TODO