Flowtype generation for queries with Inline Fragments

[ajhyndman]

This is a proposed improvement to flowtype generation for the relay compiler.

When writing queries against a union type, we use Inline Fragments to select fields from one member of the union type. In the generated flowtypes, I am seeing a single object type with optional fields.

Flow does support union types as well, however. If we are able to map GraphQL union types to Flow union types, client code we can get improved static guarantees and reduce the amount of pessimistic runtime checks that we need to execute.

e.g.

With a schema that looks like this:

type Query {
  users: [User]!
}

type User = StudentUser | TeacherUser

type StudentUser {
  name: String!
  classes: [String]!
  age: Int!
}

type TeacherUser {
  name: String!
  position: String!
  title: String!
}

If my query looks like this:

query AppQuery {
  users {
    ... on StudentUser {
      classes
      age
    }
    ... on TeacherUser {
      position
      title
    }
  }
}

I expect the generated flowtypes to look like this:

type AppQueryResponse = {|
  users: $ReadOnlyArray<?{|
    classes: $ReadOnlyArray<?string>;
    age: number;
  |} | {|
    position: string;
    title: string;
  |}>;
|}

But instead, I see this:

type AppQueryResponse = {|
  users: $ReadOnlyArray<?{|
    classes?: $ReadOnlyArray<?string>;
    age?: number;
    position?: string;
    title?: string;
  |}>;
|}

[kassens]

We currently create the first case when you include __typename in the fragment. This then allows flow to distinguish the cases in code like:

if (response.__typename === 'StudentUser') {
  console.log(users.age);
}

I found that always creating the unions sometimes makes for awkward code to actually get the values. Certainly not 100% satisfied here.

[ajhyndman]

@kassens Thanks for the tip! I'd love to see that documented somewhere.

Can you recall any scenarios in which the former case is more awkward? It seems to me that in the worst case, you could the the first case as the second when consuming without code changes, couldn't you?

I'd like to highlight another, more obvious optimisation that could be made, too.

Currently, if you write the following query:

query AppQuery {
  users {
    ... on StudentUser {
      classes
      age
    }
  }
}

I would like to see:

type AppQueryResponse = {|
  users: $ReadOnlyArray<?{|
    classes: $ReadOnlyArray<?string>;
    age: number;
  |}>;
|}

But instead I see:

type AppQueryResponse = {|
  users: $ReadOnlyArray<?{|
    classes?: $ReadOnlyArray<?string>;
    age?: number;
  |}>;
|}

[DennisKo]

We actually encountered that problem as well.

Query:

feed {
  edges {
    node {
      __typename
      ... on Resource1 {
        id
        __typename
        createdAt
      }
      ... on Resource2 {
        id
        __typename
        createdAt
      }
      ... on Resource3 {
        id
        __typename
        createdAt
      }
      ... on Resource4 {
        id
        __typename
        createdAt
      }
  }
}

Generated types:

+feed: ?{|
      +edges: ?$ReadOnlyArray<?{|
        +node: ?{|
          +__typename: "Resource1";
          +id: string;
          +createdAt: any;
          |} | {|
            // This will never be '%other', but we need some
            // value in case none of the concrete values match.
            +__typename: "%other";
          |};
          +__typename: "Resource2";
          +id: string;
        |} {|
          +__typename: "Resource3";
          +id: string;
          |} | {|
            // This will never be '%other', but we need some
            // value in case none of the concrete values match.
            +__typename: "%other";
          |};
          +__typename: "Resource4";
          +id: string;
        |} | {|
          // This will never be '%other', but we need some
          // value in case none of the concrete values match.
          +__typename: "%other";
        |};
      |}>;
    |};

Note, that fields that occur in every type (createdAt in the example) only get generated once.

[maxaggedon]

I just encountered the exact same problem as @DennisKo with a created_at field which is present in both types in a union. Only the first has the field in its generated flow type declaration...

But I'm not sure if that's the same issue, or if it is fixed in the PR #2273

[ktosiek]

@ajhyndman for TypeScript I've proposed a mixed approach: all fields are available as optional, but in concrete types you can get better types: https://github.com/relay-tools/relay-compiler-language-typescript/pull/101. Maybe something like this can work in flow too?

[ajhyndman]

@ktosiek Oh, that makes sense! Sounds quite agreeable.

[mull]

Is there any chance we'll ever see this fixed? I don't understand how this isn't a massive headache in Facebook's code base. It's almost two years ago since this issue was opened, but I see no response from the Flow team. In the blog post from January, we were told to expect more open communication but there's no evidence of that here. Come on, guys!

[alloy]

@mull More open communication does not mean they will fix issues that they themselves don’t run into, it means that if you take an active stance and want to fix this for your situation they will be more active in helping you get that to a mergeable state (if it doesn’t degrade performance on their end).

I.e. please do send a PR, perhaps you can use @ktosiek’s typescript PR as an inspiration.

[sibelius]

generating only the __typename based on fragments can be too strict for some cases

check this https://github.com/relay-tools/relay-compiler-language-typescript/issues/116

[mull]

@alloy I'm not asking for them to fix it, but rather to not go nearly 2 years without a response. Sorry if I came off as "demanding" anything!

[alloy]

@mull No worries! Mostly I just want there to be a fruitful outcome, so hopefully my explanation of the mere existence of the ticket not necessarily leading to a solution can help somebody 🙏

[dmnd]

I think this got fixed here: https://github.com/facebook/relay/commit/088afdf347582533b9956adbc5b2a5e37fe9cfaf

[stale[bot]]

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.