Better support for abstract types (unions and interfaces).

[MaartenStaa]

Represent the possible types and type names as unions in the Flow output, for more accurate typing.

This is a direct port of my work at relay-tools/relay-compiler-language-typescript#192, I'm hoping we can include this upstream to keep the two repositories in line with each other and to bring these improvements to more people.

Copy paste from the other PR: 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.

I'm looking forward to your feedback!

There are two Flow errors left due to it not understanding the value of a variable being narrowed down to an AbstractTypeID. I'm not very familiar with Flow, maybe you can help me with how to fix this.

[facebook-github-bot]

Hi @MaartenStaa!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[josephsavona]

Thanks for the PR! We agree that there are some changes and improvements that could be made to the Flow types for abstract types, so this is an area we're interested in revisiting at some point. However, some of the details of this PR are different than we had been planning, and there is a practical aspect to when we make this change and how to roll it out:

• The use of the __typename: "%other" case is intentional. The generated types must reflect the possible values that the server could return in practice: assuming no backwards-incompatible changes, that means the generated types must account for the possibility of new types appearing on the server that didn't exist when the code was compiled. With the approach taken in this diff (removing the '%other' case), users can write exhaustive switch statements w/o a default branch, when in practice the value may not be any of the listed types. In other words: the generated types here will allow users to write non-future-proof code and could lead to more bugs.
• That said, we agree that ideally we would generate type unions instead of intersections more, avoiding places where all the keys become optional.
• Practically speaking, we're in the midst of a project to completely rewrite the compiler from scratch in Rust. Until that's complete, we're trying to minimize the number of major behavioral (ie output) changes to the compilers. So this change would have to wait until the new compiler is further along, possibly later this year.
• Finally, though, there's the question of how to roll out this change. This would be a breaking change in the sense of potentially introducing new type errors to existing codebases. Even though runtime behavior is unaffected, it's still something that requires more careful rollout. We might have to temporarily support two output modes, for example.

To summarize: we sincerely appreciate the contribution but we can't immediately proceed with this change as-is. If you're interested in contributing in this area, the more pressing concern is implementing Typescript type generation in the new Rust compiler - ie porting relay-tools/relay-compiler-language-typescript to Rust. Any interest?

cc @kassens @tyao1

[MaartenStaa]

Thanks for your reply, let me reply point by point.

The use of the __typename: "%other" case is intentional. The generated types must reflect the possible values that the server could return in practice: assuming no backwards-incompatible changes, that means the generated types must account for the possibility of new types appearing on the server that didn't exist when the code was compiled. With the approach taken in this diff (removing the '%other' case), users can write exhaustive switch statements w/o a default branch, when in practice the value may not be any of the listed types. In other words: the generated types here will allow users to write non-future-proof code and could lead to more bugs.

Makes sense to me. I know there's a flag noFutureProofEnums, but that only refers to enums. Would it make sense to apply this to the "%other" case as well? Maybe renamed as noFutureProofness, or introducing a separate flag for noFutureProofTypeNames. Then it's up to the user. In my specific case the backend & frontend are in the same repository, so the odds of them getting out of sync is very small, and easy to enforce in CI as well.

That said, we agree that ideally we would generate type unions instead of intersections more, avoiding places where all the keys become optional.

Great, that's exactly what I was going for!

Practically speaking, we're in the midst of a project to completely rewrite the compiler from scratch in Rust. Until that's complete, we're trying to minimize the number of major behavioral (ie output) changes to the compilers. So this change would have to wait until the new compiler is further along, possibly later this year.

I saw the Rust rewrite project, but found it difficult to judge in what phase that project is. Are you saying the aim is to release the new compiler later this year, replacing the current Javascript-based one at the same time.

Finally, though, there's the question of how to roll out this change. This would be a breaking change in the sense of potentially introducing new type errors to existing codebases. Even though runtime behavior is unaffected, it's still something that requires more careful rollout. We might have to temporarily support two output modes, for example.

Would it make sense to roll this out with a new major version number, to indicate that there may be breaking changes? I imagine so far the Rust rewrite is mirroring the original behaviour to be able to verify more easily whether the output is correct or not. Once it reaches feature parity however there'd be room to diverge if it will be released as a major update, meaning people would be consciously opting in to the new version.

To summarize: we sincerely appreciate the contribution but we can't immediately proceed with this change as-is. If you're interested in contributing in this area, the more pressing concern is implementing Typescript type generation in the new Rust compiler - ie porting relay-tools/relay-compiler-language-typescript to Rust. Any interest?

That sounds like a fun challenge, let me see what I can cook up before heading on vacation this weekend.

[kassens]

At facebook, we need the future proof stuff as React Native clients might be months out of date and should still be able to query. I think the no-future-proof version might be actually more common outside of facebook where you might either have a web-app and you can ignore short term "website push" inconsistencies or force React Native clients to update occasionally.

I'd be pretty happy with having this an option, maybe even default to true option. I suspect the case where we keep %other wouldn't change much if at all.

The status of the Rust project is that we've just switched to using it internally for most parts, but OSS still has some outstanding work to switch. I just documented the things I could think of in #3180.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[jacob-indieocean]

I would love to have the option to not future-proof unions. For more tightly coupled client and servers, statically enforcing exhaustive union checks is very helpful. We lose the ability to do that with the widening on __typename.

That said, I'm still somewhat confused by the typing on unions and I wonder if there can be improvements made while still maintaining future-proofing.

For example, with schema:

type Success {
  result: String!
}

type Failure {
  reason: String!
}

union Result = Success | Failure

type Mutation {
  doStuff: Result!
}

and mutation

mutation DoStuffMutation {
  doStuff {
    __typename
  }
}

we get:

readonly doStuff: {
  readonly __typename: string
}

why doesn't __typename get 'Success' | 'Failure' | '%other'? Or

readonly doStuff: 
  { readonly __typename: 'Success'} | 
  { readonly __typename: 'Failure'} | 
  { readonly __typename: '%other'}

That way we can get future-proofing but also enforce exhaustive checking on the current schema.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[Jomik]

We still need this - currently interfaces and unions just result in things like __typename being a string, in typescript.
Or worse, if we do ... on Foo { __typename } it actually becomes a non-optional non-nullable "Foo" literal.