Improve type generation accuracy #192
Conversation
When using fragments, the __typename is now correctly set to a union of the remaining types. In the case where the type name is selected without the use of fragments, the value is a union of all possible types.
When selecting a __typename both on the root of the union and in a fragment, the __typename was duplicated in the output, this commit fixes that by only including the base fields that are not also selected in the concrete types.
This makes the type generation play better with the checks for "is the
__typename the only field being selected from the base type".
For example:
fragment Foo_bar on Baz {
__typename
...OtherObject_property
... on B {
__typename
value
}
}
Previously, this would result in something like { __typename: "A" | "B";
value?: string; " $fragmentRefs": "..." }
This breaks type guards in Typescript:
if (bar.__typename == "B") {
// Uh-oh, value is still possibly undefined
}
With this change, since the required fragments are tracked separately,
the resulting code looks more like this:
{ __typename: "B"; value: string; " $fragmentRefs": "..." } |
{ __typename: "A"; " $fragmentRefs": "..." }
…han the type name, and deduplicate output. The resulting type is now a union of possible types filtered by the type name, combined with all the base fields as an intersection.
|
@renanmav @n1ru4l @kastermester I've pushed two more commits. The typename in the output is now also a union when using interfaces, and I've fixed several edge cases when using fragments on types combined with selecting base fields. I'd love to hear your feedback. |
|
I will need to go proper through the test case changes and compare them to Relay's own output. Having just glanced at it - it looks to me as if we have either always had pretty broken code, or that this PR breaks some pretty crucial behavior. I think matching the behavior is pretty crucial for a couple of reasons:
So I would like to encourage you to compare the changes in the test with the output from tests in the Relay repository. This is, I think, rather important. Nice work on this so far though! :) |
|
Yes, completely agree! It's important that the output is correct. As far as I can tell it is, but I'm relatively new to Relay (and was just immediately annoyed with the relatively limited typing compared to, say, GraphQL Codegen for Apollo). I saw the Rust project in the Relay repository but it looks to be in a very early stage. I don't see an issue related to the Rust rewrite in their repo, otherwise maybe we could get in touch with the changes from this merge request, and see if it can be included upstream for the next version. If deviating from the default implementation (and its test output snapshot) is too big a deal, I could see whether I could make a parallel merge request on the Relay repository with these same changes, but for JavaScript and Flow. Let me know what you guys think. |
Without having too much knowledge of the Apollo stack I think in general the Relay team aim towards making sure that your Relay app (and by extension the typings) will be correct even when the server schema evolves. This is the reason you see things such as the
I also think this is quite a ways off. I don't think we need to do much in this regard as the Relay team are usually pretty good at providing a migration path - which we should be able to follow if we stick to their way of generating code.
This is the solution I generally advocate for if there's an issue present in both repositories. If the PR is merged on the Relay side then merging a functionally equal PR here is trivial. |
…ration-accuracy # Conflicts: # src/TypeScriptGenerator.ts # src/TypeScriptTypeTransformers.ts
|
did this get stranded completely? #190 is a pretty annoying problem |
|
Thanks for the contribution! In general for larger changes such as this we recommend discussing first to make sure we're aligned on the approach. In this case we do agree that we could output more precise types to describe queries, but the approach here doesn't fully address all the use-cases we're considering. See my comment at #190 (comment) - the plan that we're considering would change the way the runtime returns selections to namespace data in inline fragments (and possibly also fragment spreads). Given the difference in approach, we aren't likely to proceed with this exact PR. We're definitely open to feedback about how we approach this, but I would recommend discussing first before sending a large PR. |
Look at the commits for more details. Basically I have included some changes to support more accurate type output, especially regarding union types and type names.
This should also fix the case where duplicated __typename property definitions are generated, depending on where in the query or fragment the __typename property is requested.
Fixes #190, and maybe #186.