Improve type generation accuracy

[MaartenStaa]

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.

[MaartenStaa]

@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.

[kastermester]

I will need to go proper through the test case changes and compare them to Relay's own output. I am pretty swamped at work at the moment so I cannot guarentee when I will have the time. For anyone interested these tests were originally ported from RelayFlowGenerator-test.js - so it should be possible to do a comparison.

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:

1. This is pretty complex stuff, by changing one thing we might break another if we don't take great care. By relying on Relay's own way of doing things we push those concerns to the Relay team (whom have also already had to worry about these things).
2. This provides us with an easy way of verying correct results, just compare to the original.
3. As far as I can tell, the Relay team is working on a rust implemented compiler which has support for TypeScript advertised. This might mean that eventually we will have to migrate to that compiler. By sticking with the original behavior such a migration should be much easier.

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! :)

[MaartenStaa]

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.

[kastermester]

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).

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 "%other" types in the enums, to allow the server schema to add new types to a union etc. without the client code breaking (by never using the type system to assert the result is 1 of 2 types, but always having to handle that a 3rd or 4th type enters the picture).

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.

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.

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.

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.

[MrLoh]

did this get stranded completely? #190 is a pretty annoying problem

[josephsavona]

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 https://github.com/relay-tools/relay-compiler-language-typescript/issues/190#issuecomment-763964196 - 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.