[relay-compiler]: Union types - issue 2870

[morrys]

[TypeGen] Refactor discriminated union handling in visit.ts

Summary

This PR refactors how Relay TypeGen handles discriminated unions in visit.ts issue: #2870. The main goals are:

• Remove unused functions.
• Simplify and optimize should_emit_discriminated_union.
• Correctly merge common fields across concrete types and handle __typename consistently.
• Adjust test expectations to reflect the updated union and typename handling logic.

---

Changes in visit.ts

1. Remove unused function

Removed:

fn has_typename_selection(selections: &[TypeSelection]) -> bool

should_emit_discriminated_union no longer uses this function and no other code depends on it.

2. Update should_emit_discriminated_union

Before:

if by_concrete_type.is_empty() || !concrete_type.is_abstract_type() {
return false;
}

base_fields.values().all(TypeSelection::is_typename)
&& (base_fields.values().any(TypeSelection::is_typename)
|| by_concrete_type.values().all(|selections| has_typename_selection(selections)))

After:

if by_concrete_type.is_empty() || !concrete_type.is_abstract_type() || base_fields.is_empty() {
return false;
}

return base_fields.values().any(TypeSelection::is_typename);

Notes:

First change is an optimization: base_fields.is_empty() is equivalent to the previous all(...) && any(...) check.

Second change is more impactful: removed || by_concrete_type.values().all(...). Now,__typename is checked in the parent and treated as a common field.

3. Update get_discriminated_union_ast

Two major changes:

• merge concrete type fields with parent fields to correctly handle nested common fields.
• for the %other case, include all parent fields instead of only __typename. This ensures the "other" object has complete common data.

4. Test Updates

1. relay-client-id: handled as a union.

2. relay-resolver-with-output-type-client-interface: no longer treated as a union.

3. updatable tests

4. typename-on-union-with-non-matching-aliases:

• _typename moved to root.
• added usertype: __typename to prevent empty User selection.

7. typename-on-union

TypenameOutsideWithAbstractType

• sorted street alphabetically.
• removed __typename from union; now treated as a common field.

TypenameInside

• no longer treated as a union (major behavioral change).
• correct approach: move __typename outside to handle as common data.

TypenameWithCommonSelections

• notes: %other now includes all common fields, not just __typename.

export type TypenameWithCommonSelections$data = {
readonly __typename: "Page";
readonly name: string | null | undefined;
readonly username: string | null | undefined;
readonly $fragmentType: "TypenameWithCommonSelections";
} | {
readonly __typename: "User";
readonly firstName: string | null | undefined;
readonly name: string | null | undefined;
readonly $fragmentType: "TypenameWithCommonSelections";
} | {
// This will never be '%other', but we need some
// value in case none of the concrete values match.
readonly __typename: "%other";
readonly name: string | null | undefined;
readonly $fragmentType: "TypenameWithCommonSelections";
};

NEW TEST: TypenameWithNestedCommonSelections: demonstrates correct handling of nested common fields.

Input:

fragment TypenameWithNestedCommonSelections on Actor {
__typename
name
emailAddresses
address { city } # common
... on User {
firstName
address { street } # only here
}
... on Page {
username
address { country } # only here
}
}

Output:

export type TypenameWithNestedCommonSelections$data = {|
+__typename: "Page",
+address: ?{| +city: ?string, +country: ?string |},
+emailAddresses: ?ReadonlyArray<?string>,
+name: ?string,
+username: ?string,
+$fragmentType: TypenameWithNestedCommonSelections$fragmentType,
|} | {|
+__typename: "User",
+address: ?{| +city: ?string, +street: ?string |},
+emailAddresses: ?ReadonlyArray<?string>,
+firstName: ?string,
+name: ?string,
+$fragmentType: TypenameWithNestedCommonSelections$fragmentType,
|} | {|

// This will never be '%other', but we need some
// value in case none of the concrete values match.
+__typename: "%other",
+address: ?{| +city: ?string |},
+emailAddresses: ?ReadonlyArray<?string>,
+name: ?string,
+$fragmentType: TypenameWithNestedCommonSelections$fragmentType,
|};

[captbaritone]

Isn't this wrong? client_interface is of type ClientInterface and thus might have a future value added to it.

[morrys]

Could you clarify what specific case you have in mind? A concrete example or test would help. thanks

[captbaritone]

(Current types are client types which undercuts this example, but if we assume this is a server type...)

The types currently imply that type name will always be the literal ClientType. However, this JS code will be loaded in someone's browser during a long-lived session. During that time the server may ship with a new version that has an additional type that implements this interface. At that point the server might start returning a typename that is no ClientType. That's the purpose of %other. To ensure the client always handles the case that the server might start returning a type that wasn't know about when the client code was written.

This is because GraphQL has well defined "breaking change" semantics and adding a new implementor of a type is not considered a breaking change and thus the client must be able to safely handle such a change.

[morrys]

In this specific case, the issue seems to stem from how Relay RelayResolver is currently handled.

There is a transformer, generate_relay_resolvers_operations_for_nested_objects, which previously ensured that __typename was generated. This allowed the type to be correctly detected by the condition:

by_concrete_type.values().all(|selections| has_typename_selection(selections))

Looking a bit deeper, it seems feasible to remove the logic responsible for generating __typename from this transformer and instead introduce a dedicated transformer that handles the new behavior.

I tried building a draft implementation of a transformer for this, and I was able to correctly produce %other for pop_star_game, whilepop_star_name is still being ignored. I’m not entirely sure whether %other is actually required for that case as well.

I’ll continue looking into this transformer. From testing a few other cases, it seems to handle unions correctly without changes to the existing code, so it could be a good fourth option.

• Support the old heuristic
• Add a feature flag which enables users to opt back into the old behavior
• Add a codemod that inserts a top-level __typename in all the places where we were previously emitting a discriminated union.
• Add Generate Typename Field Transform

[captbaritone]

See above about this being a breaking change.

[captbaritone]

Previously

... on SomeConcreteType {
  __typename
  someField
}
... on SomeOtherConcreteType {
  __typename
  anotherField
}

Would emit a discriminated union. Now it won't. That's going to be a big breaking change for a lot of users. Is there a reason we have to lose that?

[morrys]

this isn’t a blocking issue—more about keeping things clean and having clearer control over when the discriminated union is emitted, especially since this is a common field shared across types.

It is technically a breaking change, but since it only affects typing/compilation, fixes should be guided by type errors and shouldn’t impact runtime logic.

That said, if this is a concern, I can add the condition by_concrete_type.values().all(|selections| has_typename_selection(selections)), resulting in:

return base_fields.values().any(TypeSelection::is_typename)
    || by_concrete_type.values().all(|selections| has_typename_selection(selections));

[captbaritone]

I think your rational is fine, but practically speaking we'll need some smooth migration path. Some options:

1. Support the old heuristic
2. Add a feature flag which enables users to opt back into the old behavior
3. Add a codemod that inserts a top-level __typename in all the places where we were previously emitting a discriminated union.

[morrys]

@captbaritone see the comment on the client interface

[morrys]

I added the generate_typename_union transform, which automatically inserts __typename into selection sets when needed for type generation, especially for abstract types with concrete refinements.

This is consistent with the new should_emit_discriminated_union logic: the discriminant is now explicitly available in base_fields.

This simplifies usage on the user side: it is no longer necessary to manually add __typename in fragments or queries, since the compiler will insert it automatically in the correct place when needed.

This avoids a breaking change and does not require updating existing queries, nor introducing feature flags or codemods.

In this commit you will notice changes in several generated and/or expected files. This is expected, as discriminated unions are now correctly generated even in cases where __typename was not explicitly present in the query.

@captbaritone let me know what you think, and feel free to take another look when you have time 👍

[meta-codesync]

@tyao1 has imported this pull request. If you are a Meta employee, you can view this in D100882850.

[tyao1]

Why are these validations removed?

[morrys]

Hi @tyao1,

right now the validation only allows fields with inline fragments, like this:

query TestQuery @updatable {
  node(id: 4) {
    ... on Page {
      name
    }
    ... on Comment {
      firstName
    }
  }
}

however, with the new union logic, including __typename should also be valid:

query TestQuery @updatable {
  node(id: 4) {
    __typename
    ... on Page {
      name
    }
    ... on Comment {
      firstName
    }
  }
}

because of that, the current check is no longer correct, so I removed it.

Alternatively, we could relax it to also allow __typename fields, what do you think?