Relay-compiler-language-typescript: Clarity on typing common fields on interfaces

[thoughtbyte]

The #103 enhancement in 4.1.0 causes a lot of TS errors we haven't seen before and I'm not sure where the confusion/error rests.

Here is a small example from our codebase:

We have a query like so:

node: graphql`
  fragment NodeLink_node on Node {
    __typename
    ... on Publishable {
      speakingId
    }
    ... on ExternalResource {
      url
    }
    ... on Video {
      listMemberships_series: listMemberships(byListType: "series") {
        list {
          speakingId
        }
      }
    }
  }
`,

The emitted type prior to 4.1.0:

export type NodeLink_node = {
    readonly __typename: string;
    readonly speakingId?: string | null;
    readonly url?: string | null;
    readonly listMemberships_series?: ReadonlyArray<({
        readonly list: ({
            readonly speakingId: string | null;
        }) | null;
    }) | null> | null;
    readonly " $refType": NodeLink_node$ref;
};

With the #103 enhancement, the emitted type is now:

export type NodeLink_node = {
    readonly __typename: string;
    readonly speakingId?: string | null;
    readonly url?: string | null;
    readonly listMemberships_series?: ReadonlyArray<{
        readonly list: {
            readonly speakingId: string | null;
        } | null;
    } | null> | null;
    readonly " $refType": NodeLink_node$ref;
} & ({
    readonly __typename: "ExternalResource";
    readonly url: string | null;
} | {
    readonly __typename: "Video";
    readonly listMemberships_series: ReadonlyArray<{
        readonly list: {
            readonly speakingId: string | null;
        } | null;
    } | null> | null;
} | {
    /*This will never be '% other', but we need some
    value in case none of the concrete values match.*/
    readonly __typename: "%other";
});

But this new emitted type throws an error for the following check we perform in the component:

if (node.__typename === 'Article') {
  // do some stuff
}

The error: TS2367: This condition will always return 'false' since the types '"Video" | "%other"' and '"Article"' have no overlap.

I don't know if this is a bug, or if our query is in some way an anti-pattern? Any advice is appreciated, and thank you for your work on this package!

[sibelius]

your node can only be Video, ExternalResource?

where is Article in your Node?

can you share part of your schema?

[kastermester]

Also - how does the same query with the same schema behave when using the type generation for flow provided by relay itself?

[thoughtbyte]

Hi @kalekseev , the flow generated type:

export type NodeLink_node = {|
  +__typename: string,
  +speakingId?: ?string,
  +url?: ?string,
  +listMemberships_series?: ?$ReadOnlyArray<?{|
    +list: ?{|
      +speakingId: ?string
    |}
  |}>,
  +$refType: NodeLink_node$ref,
|};
export type NodeLink_node$data = NodeLink_node;
export type NodeLink_node$key = {
  +$data?: NodeLink_node$data,
  +$fragmentRefs: NodeLink_node$ref,
};

@sibelius the node can be an Article. I'm not sure what you mean where is it in the Node? I can share any part of the schema, not sure what is best to share, it's pretty long.

[sibelius]

in this fragment there is no Article

fragment NodeLink_node on Node {
    __typename
    ... on Publishable {
      speakingId
    }
    ... on ExternalResource {
      url
    }
    ... on Video {
      listMemberships_series: listMemberships(byListType: "series") {
        list {
          speakingId
        }
      }
    }
  }

it looks like typescript emitted types are more strict them flowtype ones

[thoughtbyte]

I guess that's the root of my question. Why would Article have to be in there? Why is this level of strictness introduced?

Here's the use case fleshed out a bit. Somewhere higher up we query for a list of nodes. They could be of any type. We map through them and render a NodeLink for each one, passing in the node. The NodeLink normally just renders a found Link, but for some types, like article in this case, we render something different inside NodeLink, hence the check for __typename. Maybe I'm missing something or not understanding some idiom of relay/graphql, but this has always worked. This new emitted type breaks that and I'm not sure why. I.e., I understand why on the type level, but I don't really understand why this strict type is emitted. It seems overly restrictive to my naive understanding.

Also, thanks again for your help with this, I very much appreciate it.

[sibelius]

The core of relay fragments is that each component should only rely on the data they ask for (data masking)

If your component are not asking for node of type Article, you should not be able to see this data in your component

as you are not asking for any Article data how can you render an Article?

[ktosiek]

Would adding an empty fragment on Article help? Something like ...on Article { __typename }.

I don't think TypeScript allows typing the '%other' case as just string and still keeping the tagged union behaviour (I've tried and failed). It would be great to find a way to support your use case, as the API might return a type name that was not present in the query (for interfaces).

[tomconroy]

@ktosiek is correct: Node here is an interface, the concrete node type may be an Article. It does not matter that we don't query for Article-specific fields, since we only care about the type name:

if (node.__typename === 'Article') return `/article/${node.speakingId}`;

[sibelius]

why not query all the types that you going to need?

[tomconroy]

It's a reasonable workaround, but the generated types don't quite match the actual behavior of GraphQL – I guess this is the source of the confusion

[alloy]

As @sibelius mentioned, the difference is that the way to think with Relay is that you only have access to what you queried for–which is then reflected in the types.

[tomconroy]

I understand now, it's reasonable, thank you both!

[thoughtbyte]

Thank you for all the help everyone! Very much appreciated.

[alloy]

🙌

[NickDubelman]

This seems weird to me. I'll submit something upstream but I just created a reproduction that shows that for both Flow and TS, you have to do this:

fragment App2_animal on Animal {
  __typename
  ... on Cat {
    id
    name
    age
    lives
  }
  ... on Dog {
    id
    name
    age
    breedDescription
  }
}

instead of

fragment App_animal on Animal {
  __typename
  id
  name
  age
  ... on Cat {
    lives
  }
  ... on Dog {
    breedDescription
  }
}

And this seems intentional, because in past versions, it wasn't the case? Or is this a bug?

[ktosiek]

@NickDubelman I went there, and until it works in upstream I wouldn't bet on sensible common fields handling to be supported here - different algorithms for TS and Flow adds more maintenence work.

The story, as I remember it, goes like this:

• relay-compiler-language-typescript (let's call it Relay-TS) started with algorithm based on the one in Relay, which only handled __typename as a common field. Everything else had to be duplicated, or put into separate fragments - Relay API likes fragments.
• Around v4.0 I've made a PR (#101) to make the types looser in Relay-TS, by making all subtype-specific fields optional on the fragment + generating a union tagged with __typename (this is IIRC, and as far as I understand that code now, but that's pretty much how I used those patches on a client project).
• Then around v6.0 the whole Relay algorithm was ported to Relay-TS, which brings us back to the current behaviour: #127

This thread in Relay talks about the same issue: https://github.com/facebook/relay/issues/2039. The last comment suggests that it got fixed upstream, if so - maybe it's time for another Relay -> Relay-TS port?