-
-
Notifications
You must be signed in to change notification settings - Fork 18
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 union types to schema #309
add union types to schema #309
Conversation
The schema is reasonable 👍 Make sure to both update the adapter and the index lookup optimizations, otherwise we might end up in a situation where index lookups fail to find unions that otherwise get returned when the optimization isn't used. Lmk if you get stuck on anything or would like to chat about anything. |
Sounds good. I'm basically just grepping for |
00bde00
to
7602fbc
Compare
Ok, the meat of it is there. I'm not too familiar with the codebase, do you know if there are any specific union-related tests I should add, off the top of your head? |
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.
do you know if there are any specific union-related tests I should add
I'd add a test crate with some unions, and then make sure:
- querying for all unions and their fields returns correct data
- querying for union importable paths returns correct data (even with re-exports)
- querying unions by importable path returns correct data (this will use the fast paths and use an index lookup optimization)
dd87126
to
0e562b7
Compare
Hmm, it actually seems less disruptive to just test
can be tested in the |
This test case is also included in this commit as it exposed the bug.
Let me know when this is ready for another review! |
btw, make sure to check out that test case, because I'm not sure it's testing quite what we might be assuming it's testing here |
You're right, I did some more digging into the test crates, and that one is testing something else. I've actually come to your original conclusion post-dig, which is that it's just simpler to have a crate for unions. Because structs and enums were there from the beginning, there are no specific tests for them, rather they're just used everywhere. Since unions are new their introduction might cause some bugs so they can get ~their own crate~. I've gotta a pretty busy day today, so I might not be able to finish this up today, but I'll try to get it done by the weekend! |
Sounds good! Thanks for setting expectations explicitly as well, it'll help me plan my own day as well. |
Ok, added a test crate. I think I've covered everything (importing, visibility, field info), but not sure. |
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.
Awesome, ship it! 🚀
* add union types to schema * add union support to adapter * avoid extra indirection when resolving union edge * add union to supported item kinds * resolve __typename of unions This test case is also included in this commit as it exposed the bug. * add unions to some glob test cases * add tests
* add union types to schema * add union support to adapter * avoid extra indirection when resolving union edge * add union to supported item kinds * resolve __typename of unions This test case is also included in this commit as it exposed the bug. * add unions to some glob test cases * add tests
I've used a bit of magic automation to port this PR to |
* add union types to schema * add union support to adapter * avoid extra indirection when resolving union edge * add union to supported item kinds * resolve __typename of unions This test case is also included in this commit as it exposed the bug. * add unions to some glob test cases * add tests Co-authored-by: Felix Prasanna <[email protected]>
* add union types to schema * add union support to adapter * avoid extra indirection when resolving union edge * add union to supported item kinds * resolve __typename of unions This test case is also included in this commit as it exposed the bug. * add unions to some glob test cases * add tests Co-authored-by: Felix Prasanna <[email protected]>
union
to schemaFixes #36.