Adding __typename on a interface field generates generates string type instead of union type with constant strings #190

n1ru4l · 2020-05-31T14:48:35Z

Schema Types:

type DiceRollCloseParenNode implements DiceRollDetail {
  content: String!
}

type DiceRollConstantNode implements DiceRollDetail {
  content: String!
}

interface DiceRollDetail {
  content: String!
}

type DiceRollDiceRollNode implements DiceRollDetail {
  content: String!
  min: Float!
  max: Float!
  rollResults: [DiceRollResult!]!
}

type DiceRollOpenParenNode implements DiceRollDetail {
  content: String!
}

type DiceRoll {
  result: Float!
  detail: [DiceRollDetail!]!
}

export const FormattedDiceRoll = createFragmentContainer(DiceRollRenderer, {
  diceRoll: graphql`
    fragment formattedDiceRoll_diceRoll on DiceRoll {
      result
      detail {
        __typename
        content
        ... on DiceRollDiceRollNode {
          content
          rollResults {
            dice
            result
            category
          }
        }
      }
    }
  `,
});

Generated Type:

export type formattedDiceRoll_diceRoll = {
    readonly result: number;
    readonly detail: ReadonlyArray<{
        readonly __typename: string;
        readonly content: string;
        readonly rollResults?: ReadonlyArray<{
            readonly dice: string;
            readonly result: number;
            readonly category: DiceRollCategory;
        }>;
    }>;
    readonly " $refType": "formattedDiceRoll_diceRoll";
};

Expected Generated Type:

export type formattedDiceRoll_diceRoll = {
    readonly result: number;
    readonly detail: ReadonlyArray<
      {
        readonly __typename: "DiceRollCloseParenNode" | "DiceRollOpenParenNode" | "DiceRollConstantNode";
        readonly content: string;
      } |
      {
        readonly __typename: "DiceRollDiceRollNode"
        readonly content: string;
        readonly rollResults: ReadonlyArray<{
            readonly dice: string;
            readonly result: number;
            readonly category: DiceRollCategory;
        }>;
      }
    }>;
    readonly " $refType": "formattedDiceRoll_diceRoll";
};

HOWEVER:

The following will generates the correct types:

Fragment Container:

export const FormattedDiceRoll = createFragmentContainer(DiceRollRenderer, {
  diceRoll: graphql`
    fragment formattedDiceRoll_diceRoll on DiceRoll {
      result
      detail {
        ... on DiceRollOperatorNode {
          __typename
          content
        }
        ... on DiceRollConstantNode {
          __typename
          content
        }
        ... on DiceRollOpenParenNode {
          __typename
          content
        }
        ... on DiceRollCloseParenNode {
          __typename
          content
        }
        ... on DiceRollDiceRollNode {
          __typename
          content
          rollResults {
            dice
            result
            category
          }
        }
      }
    }
  `,
});

Generated Code:

export type formattedDiceRoll_diceRoll = {
    readonly result: number;
    readonly detail: ReadonlyArray<{
        readonly __typename: "DiceRollOperatorNode";
        readonly content: string;
    } | {
        readonly __typename: "DiceRollConstantNode";
        readonly content: string;
    } | {
        readonly __typename: "DiceRollOpenParenNode";
        readonly content: string;
    } | {
        readonly __typename: "DiceRollCloseParenNode";
        readonly content: string;
    } | {
        readonly __typename: "DiceRollDiceRollNode";
        readonly content: string;
        readonly rollResults: ReadonlyArray<{
            readonly dice: string;
            readonly result: number;
            readonly category: DiceRollCategory;
        }>;
    } | {
        /*This will never be '%other', but we need some
        value in case none of the concrete values match.*/
        readonly __typename: "%other";
    }>;
    readonly " $refType": "formattedDiceRoll_diceRoll";
};

But this is how I expect union types to work. IMHO interface fields should not require that much boiler plate code.

GraphQL Codegen supports generating the "correct" interface selection set typings.

renanmav · 2020-06-14T22:37:19Z

Maybe I’m missing something, but shouldn’t you declare the union type within your schema?

Something like:

union DiceRollDetail =
    DiceRollDiceRollNode
  | DiceRollConstantNode
  | DiceRollCloseParenNode

type DiceRoll {
  result: Float!
  detail: [DiceRollDetail!]!
}

n1ru4l · 2020-06-15T06:03:57Z

@renanmav Why use a union type instead of an interface type? It makes it utterly complicated when selecting only the shared fields (such as content in this example).

Interface

fragment formattedDiceRoll_diceRoll on DiceRoll {
  result
  detail {
    __typename
    content
  }
}

vs

Union

fragment formattedDiceRoll_diceRoll on DiceRoll {
  result
  detail {
    ... on DiceRollOperatorNode {
      __typename
      content
    }
    ... on DiceRollConstantNode {
      __typename
      content
    }
    ... on DiceRollOpenParenNode {
      __typename
      content
    }
    ... on DiceRollCloseParenNode {
      __typename
      content
    }
    ... on DiceRollDiceRollNode {
      __typename
      content
    }
  }
}

renanmav · 2020-06-15T11:58:43Z

I’m not very experienced with union types, but the last snippet seems more explicit, and one of relay’s purposes is explicit data requirements.

kastermester · 2020-06-15T12:01:20Z

How does the generated code and your expected output align up with how Relay for flow works? I know we've had some issues regarding this in the past, but I think we should generate the code the same way the official implementation works (I'm not saying that is what is happening now, but there has been bugs both in the official implementation and in ours).

Can you do comparisons with the flow based implementation and see if there's any differences there? If there is then it is definitely a bug that we should look into fixing.

maraisr · 2021-01-18T02:41:33Z

@n1ru4l I love this idea! Would make conditional's type-safe as well.

If we get a consensus we should look at implementing it.

@kassens @josephsavona @jstejada can you give us some guidance if this is even a good idea, as we'd love this to make into relay-compiler (rust) as well.

josephsavona · 2021-01-20T21:36:06Z

Until recently this wasn't feasible, since Relay didn't know at runtime which types implement which interfaces. We changed that with the new precise type refinement feature, so that we dynamically query this information as necessary. We expect to enable that feature by default in an upcoming release at which point we can start generating more precise types for interfaces (ie as suggested here, instead of making all fields on the interface result value optional, generate a union with non-optional fields). However, we're also looking at alternatives to __typename for doing refinement in product code. Ie instead of checking typename, we could assign the result of a type refinement (... on SomeType { selection }) to a named object so that you could do if (object.SomeType) { object.SomeType.selection }.

So: we're not quite ready to do this yet, but agree w the direction.

embpdaniel · 2021-05-13T16:28:59Z

@josephsavona Can you give us any update on this and the open PR #192 that seems to resolve this?

josephsavona · 2021-05-14T13:54:05Z

This is in our backlog. It's a major change to the way we produce types and we can't simply flip a switch here, we need to figure out an upgrade path.

embpdaniel · 2021-05-14T15:15:41Z

Thanks for the info @josephsavona keep us posted 😀

jasonkuhrt · 2021-11-10T17:15:49Z

@josephsavona Our team our Prisma would be interested in contributing a solution here. Let me know what potential there is for that, we use result types in our API and so rely on this feature heavily (we're migrating from genql).

n1ru4l changed the title ~~Adding __typename on a interface field generates generates string type~~ Adding __typename on a interface field generates generates string type instead of union type with constant strings May 31, 2020

MaartenStaa linked a pull request Jun 12, 2020 that will close this issue

Improve type generation accuracy #192

Open

zerkalica mentioned this issue Dec 1, 2021

Что не так с GraphGL nin-jin/HabHub#21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding __typename on a interface field generates generates string type instead of union type with constant strings #190

Adding __typename on a interface field generates generates string type instead of union type with constant strings #190

n1ru4l commented May 31, 2020 •

edited

renanmav commented Jun 14, 2020

n1ru4l commented Jun 15, 2020 •

edited

renanmav commented Jun 15, 2020 •

edited

kastermester commented Jun 15, 2020

maraisr commented Jan 18, 2021

josephsavona commented Jan 20, 2021

embpdaniel commented May 13, 2021

josephsavona commented May 14, 2021

embpdaniel commented May 14, 2021

jasonkuhrt commented Nov 10, 2021

Adding __typename on a interface field generates generates string type instead of union type with constant strings #190

Adding __typename on a interface field generates generates string type instead of union type with constant strings #190

Comments

n1ru4l commented May 31, 2020 • edited

renanmav commented Jun 14, 2020

n1ru4l commented Jun 15, 2020 • edited

renanmav commented Jun 15, 2020 • edited

kastermester commented Jun 15, 2020

maraisr commented Jan 18, 2021

josephsavona commented Jan 20, 2021

embpdaniel commented May 13, 2021

josephsavona commented May 14, 2021

embpdaniel commented May 14, 2021

jasonkuhrt commented Nov 10, 2021

n1ru4l commented May 31, 2020 •

edited

n1ru4l commented Jun 15, 2020 •

edited

renanmav commented Jun 15, 2020 •

edited