typename discriminator not properly set if fragment on object type is spread onto union

[tobias-tengler]

 graphql`
  fragment someUnion on Union {
    __typename
    ...object1
    ...object2
  }
`;

graphql`
  fragment object1 on Object1 {
    field1
  }
`;

graphql`
  fragment object2 on Object2 {
    field2
  }
`;

The above fragments currently leads to the following TypeScript definition for the someUnion fragment:

export type someUnion$data = {
  readonly __typename: string;
  readonly " $fragmentSpreads": FragmentRefs<"object1" | "object2">;
  readonly " $fragmentType": "someUnion";
};

I would've expected it to be the following:

export type someUnion$data = {
  readonly __typename: "Object1";
  readonly " $fragmentSpreads": FragmentRefs<"object1">;
  readonly " $fragmentType": "someUnion";
} | {
  readonly __typename: "Object2";
  readonly " $fragmentSpreads": FragmentRefs<"object2">;
  readonly " $fragmentType": "someUnion";
} | {
  readonly __typename: "%other";
  readonly " $fragmentType": "someUnion";
};

To achieve the same result you currently have to define your fragment like this:

fragment someUnion on Union {
  __typename
  ... on Object1 {
    ...object1
  }
  ... on Object2 {
    ...object2
  }
}

Having to use an additional inline fragment seems unnecessary, since the fragments are already defined on a concrete object type.

[captbaritone]

Understanding when a specific fragment matched is a larger problem which this enhancement could help address but would not fully solve since fragments on abstract types/interfaces would still need to be typed as string. For anyone coming across this issue who is unfamiliar, as a rule we don't materialize all possible concrete types since in some schemas (ours) the number of concrete types may be vast.

Instead, to solve this problem we've leaned into requiring @alias on fragment spreads which only conditionally match. This materializes the fragment spreads as a nullable property which can (must!) be checked before use.

See https://relay.dev/docs/next/guides/alias-directive/#enforced-safety

With that problem more broadly solved, doses this typing still feel worth improving? I do agree with your assessment that we could be generating more specific types in this case, I'm just not sure what complexity would be involved in enabling it.

[tobias-tengler]

I like @alias generally, but I feel like there are some cases where it's a bit cumbersome.
We have for instance a blog page, where we've modeled the content as a list of unions, with each union representing a type of section. We then have one "renderer" component which has a fragment on the union and spreads the fragment for each possible union member. We then do a switch case based on the __typename and render the appropriate component for each section and pass down the fragment reference. I feel like @alias wouldn't necessarily lead to easier understandable code in such a case and I feel like we'd prefer the switch case style.

I understand the concern for the potentially added complexity though.
A colleague and I wanted to spend some time end of this or next week to explore the issue and maybe we can come up with a relatively simple solution :)

[captbaritone]

I can see that a switch could be better than a series of nullable properties for modeling the behavior you have there.

If I recall correctly, you logic for deciding how/when to generate a discriminated union vs an object with optional properties is complex and not necessarily rigorous. If you want to step all the way back to considering this problem from scratch, I'd be interested in hearing what if any solution you come up with that could feel more rigorous or easier to reason about.

It's also perhaps noting that if we did have cases where we could confidently understand that we would generate a discriminated union, we could avoid requiring @alias in those places. However, we would need to be sure we never get out of sync. In other words, if the @alias validation thinks we will emit a discriminated union, but we don't actually, that would create a typehole which can be very dangerous.