Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mark-dangerous-conditional-fragment-spreads Codemod suggestion #4901

Closed
jaroslav-kubicek opened this issue Jan 31, 2025 · 2 comments
Closed

Comments

@jaroslav-kubicek
Copy link
Contributor

jaroslav-kubicek commented Jan 31, 2025

Hi 👋

We're grateful for mark-dangerous-conditional-fragment-spreads Codemod but I was wondering if it can be adjusted in the following situation:

query editTeamspaceMembersQuery($id: ID!) {
  node(id: $id) {
    __typename
    ... on Teamspace {
      visibilityType
      ...memberships
    }
  }
}

fragment memberships on Teamspace {
  id
  name
  memberships(first: 10) {
    edges { node { id } }
  }
}

Fragment spread memberships is marked there as dangerous, but that only applies if we leave out __typename:

export type editTeamspaceMembersQuery$data = {
  readonly node: {
    readonly visibilityType?: TeamspaceVisibilityType;
    readonly " $fragmentSpreads": FragmentRefs<"memberships">;
  } | null | undefined;

But when we explicitly ask for __typename, which we do and even demand doing so by custom ESLint rule, Relay compiler generates the following types that should be safe:

export type editTeamspaceMembersQuery$data = {
  readonly node: {
    readonly __typename: "Teamspace";
    readonly visibilityType: TeamspaceVisibilityType;
    readonly " $fragmentSpreads": FragmentRefs<"memberships">;
  } | {
    // This will never be '%other', but we need some
    // value in case none of the concrete values match.
    readonly __typename: "%other";
  } | null | undefined;

As we're using interfaces a lot, this codemod produces a lot of changes, requiring us to use @alias much more often than needed, since it's already covered by checking __typename. Is this something that can be adjusted?

@captbaritone
Copy link
Contributor

Currently the logic which determines if we emit a discriminated union is very complicated, and not something other parts of the compiler (like this validation) can safely predict. Furthermore, this would create "action at a distance" where changes in one fragment could confusingly cause the type emission to change and trigger the validation error in a totally different fragment.

While I see how it would be desirable, I don't think there's a practical way for us to achieve this today, especially without breaking encapsulation.

@jaroslav-kubicek
Copy link
Contributor Author

Ok, thanks @captbaritone for the insight.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants