Relay-compiler-language-typescript: Union types

[artola]

@alloy It is a heads up, because I found some problems after upgrading to v5, it is breaking with union types, further investigation required.
My code and queries are too complex, plus not disclosable, here an example using star-wars and comparing the output with v4.13.0 vs v5

Gist (generated code comparison): https://gist.github.com/artola/684f4835328993c7dff2663f0297ab6e
Repo (full example): https://github.com/artola/rclt-bug

Is it intended or bug? I mean, am I missing some new config and so this difference?

[alloy]

Looping in @kastermester, this is likely related to https://github.com/relay-tools/relay-compiler-language-typescript/pull/127

[kastermester]

Can you try to see what flow types is generated by the flow type generation?

I’ve noticed some weird gotchas around the way it works currently, but they are all replicated in the flow type generation. I’ve not wanted to look more into it for now as relay v6 introduces a ton of new features and this changes the way the code around this is structured quite a bit.

Do note that where you put the __typename selection can change the generated types a fair bit.

[artola]

Can you try to see what flow types is generated by the flow type generation?

I’ve noticed some weird gotchas around the way it works currently, but they are all replicated in the flow type generation. I’ve not wanted to look more into it for now as relay v6 introduces a ton of new features and this changes the way the code around this is structured quite a bit.

Do note that where you put the __typename selection can change the generated types a fair bit.

@kastermester I did it with Flow v4, v5 and v6 (no difference) and it looks as:

https://gist.github.com/artola/684f4835328993c7dff2663f0297ab6e#file-with-flow-v5

I guess that the TS v4 with union might be a bit off in comparison with Flow, but it was super powerful, and now it seems not safe anymore.

Note: from a TS point of view, these types should be implemented using a discriminated union based in the __typename. As you can see in the repo, the compiler is re-writing the query and adding the __typename we should continue to use it as in v4 was done ;)
Without unions we are forced to write type guards all over the place, a problem when we deal with many fields, non sense.

/*
query trialQuery {
  hero {
    __typename
    name
    ... on Human {
      homePlanet
    }
    ... on Droid {
      primaryFunction
    }
  }
}
*/

[kastermester]

How did the types that v4 was outputting benefit you? As far as I can see the unions being generated was of no real help code wise?

The flow types and the TS types looks comparable/identical which is what we’ve been aiming at, so good news is there’s not a bug here as far as I can spot (at least not one that is also not a problem in relay itself).

Can you try moving the __typename field around? (Ie. inside the Human and Droid in-line fragment spreads) and see if that generate a different type for you that is more useful?

[artola]

@kastermester Moving __typename inside Human or Droid makes the same sense that add it to Hero (and it is done automatically by the relay re-writer).

There is some bug ongoing here, what happens when I add it manually to the query and the typings generated are WRONG. Why? because we queried by "heroes" including some fields that are mutex for "droid" and "human" but in the typings we get altogether mixed and now the readonly __typename: "Human"; ignoring the "droid".

What we have in v4 with unions what perfect for TS.

`` // query graphql
query trialQuery {
hero {
name
... on Human {
__typename
homePlanet
}
... on Droid {
__typename
primaryFunction
}
}
}
`;

// generated
import { ConcreteRequest } from "relay-runtime";
export type trialQueryVariables = {};
export type trialQueryResponse = {
readonly hero: {
readonly name: string;
readonly __typename: "Human";
readonly homePlanet?: string | null;
readonly primaryFunction?: string;
};
};
export type trialQuery = {
readonly response: trialQueryResponse;
readonly variables: trialQueryVariables;
};

/*
query trialQuery {
hero {
__typename
name
... on Human {
__typename
homePlanet
}
... on Droid {
__typename
primaryFunction
}
}
}
*/```

[artola]

@kastermester Example why unions of v4 make a lot of sense:

In v4 this code will not compile, because is not possible in Human to have primaryFunction (discriminated union + type guard).

if (data.__typename === 'Human') {  // <== type guard
  console.log(data.primaryFunction); // <== error
}

But in v5 the code will compile and print undefined.

```ts
if (data.__typename === 'Human') {  // <== type guard
  console.log(data.primaryFunction); // ==> undefined
}

Imagine that in place of having 1 field of difference, we have 10 of them on each side. An union works perfectly, with 1 type guard we are able to narrow the type of the fields inside the block, while in the other case the type system does not help at all, here a type guard is just a JS block and we need to check each field for undefined.

Am I missing something?

[kastermester]

@kastermester Moving __typename inside Human or Droid makes the same sense that add it to Hero (and it is done automatically by the relay re-writer).

What Relay adds in its rewriting phase should not impact the types generated. If you wish to use __typename you need to add a selection for it. Did your original queries in the gist not include this?

There is some bug ongoing here, what happens when I add it manually to the query and the typings generated are WRONG. Why? because we queried by "heroes" including some fields that are mutex for "droid" and "human" but in the typings we get altogether mixed and now the readonly __typename: "Human"; ignoring the "droid".

This part is absolutely a bug - now we just need to figure out if this is isolated to the TypeScript plugin or if it is existing in Relay (flow) as well.

What we have in v4 with unions what perfect for TS.

It might be good for some use cases - but it was fixed due to issues like #120.

We need to figure out if these issues you are hitting are replicated in Relay itself as well. If that is the case (and they are actual bugs like the one you mentioned) we should get these bugs filed in Relay - and then we can port their fixes over as well. We need to keep the codebases as easy to port as possible - such that we only deal with the differences between TS and Flow. If we don't do it this way then we'll have trouble keeping up with new features of Relay itself.

My point is not that the v5 stuff is ideal (I've found quite a few oddities with this but have not had the time to file a proper bug in Relay about it) - but simply that we need to strive towards the same behavior as much as we can.

For reference, here is an extracted fragment and its generated file from our codebase CRMCompanyProjectRelation is a union type:

fragment CRMCompanyProjectRelationType_projectRelation on CRMCompanyProjectRelation
    @argumentDefinitions(locale: { type: "Locale!" }) {
    __typename
    ... on CRMCompanyProjectRelationCustomer {
        project {
            __typename
        }
    }
    ... on ProjectCollaborationPartner {
        collaborationType {
            id
            localizedText(locale: $locale)
        }
    }
}

Generated type:

import { ReaderFragment } from "relay-runtime";
declare const _CRMCompanyProjectRelationType_projectRelation$ref: unique symbol;
export type CRMCompanyProjectRelationType_projectRelation$ref = typeof _CRMCompanyProjectRelationType_projectRelation$ref;
export type CRMCompanyProjectRelationType_projectRelation = {
    readonly __typename: "CRMCompanyProjectRelationCustomer";
    readonly project: {
        readonly __typename: string;
    };
    readonly " $refType": CRMCompanyProjectRelationType_projectRelation$ref;
} | {
    readonly __typename: "ProjectCollaborationPartner";
    readonly collaborationType: {
        readonly id: string;
        readonly localizedText: string | null;
    } | null;
    readonly " $refType": CRMCompanyProjectRelationType_projectRelation$ref;
} | {
    /*This will never be '%other', but we need some
    value in case none of the concrete values match.*/
    readonly __typename: "%other";
    readonly " $refType": CRMCompanyProjectRelationType_projectRelation$ref;
};

As you can see - it is possible to get the v5 generation to generate usable types this way. But it is of utmost importance that the __typename selection is put in the right place. Also I have not had any experience with this directly on an operation and not on a fragment - perhaps the behaviors here do not match.

[kastermester]

if (data.__typename === 'Human') {  // <== type guard
  console.log(data.primaryFunction); // <== error
}

I can't see how this if-statement should do anything useful for v4. Below is the types from your Gist for v4 - which contains no real useful definitions of __typename that would help this.

export type trialQueryResponse = {
    readonly __typename: string;
    readonly hero: {
        readonly name: string;
        readonly homePlanet?: string | null;
        readonly primaryFunction?: string;
    } & ({
        readonly homePlanet: string | null;
    } | {
        readonly primaryFunction: string;
    } | {
        /*This will never be '% other', but we need some
        value in case none of the concrete values match.*/
        readonly __typename: "%other";
    });
};

[kastermester]

I have now checked out your repo and tried messing around with it. If I add the __typename selection like this:

  query trialQuery {
    hero {
      __typename
      name
      ... on Human {
        homePlanet
      }
      ... on Droid {
        primaryFunction
      }
    }
  }

I get a usable type definition under v4 like this:

export type trialQueryResponse = {
    readonly hero: {
        readonly __typename: string;
        readonly name: string;
        readonly homePlanet?: string | null;
        readonly primaryFunction?: string;
    } & ({
        readonly __typename: "Human";
        readonly homePlanet: string | null;
    } | {
        readonly __typename: "Droid";
        readonly primaryFunction: string;
    } | {
        /*This will never be '% other', but we need some
        value in case none of the concrete values match.*/
        readonly __typename: "%other";
    });
};

And under v5 a not-very-helpful type:

export type trialQueryResponse = {
    readonly hero: {
        readonly __typename: string;
        readonly name: string;
        readonly homePlanet?: string | null;
        readonly primaryFunction?: string;
    };
};

Sadly this behavior is replicated in relay itself.

Good news is that if I change the query a bit we instead get the expected types:

  query trialQuery {
    hero {
      __typename
      ... on Human {
        name
        homePlanet
      }
      ... on Droid {
        name
        primaryFunction
      }
    }
  }

This generates (under v5):

```ts
export type trialQueryResponse = {
readonly hero: {
readonly __typename: "Human";
readonly name: string;
readonly homePlanet: string | null;
} | {
readonly __typename: "Droid";
readonly name: string;
readonly primaryFunction: string;
} | {
/This will never be '%other', but we need some
value in case none of the concrete values match./
readonly __typename: "%other";
};
};
````

I'm not quite sure what the bug in Relay is here. I'm not sure they actually want to allow you to select name without a type guard in the query - as the server could in theory change its schema such that the selection would be invalid - so perhaps a missing validation is the error here. If that is not the case - then obviously there's an error that triggers wrong code generation when there's common fields selected for unions.

I hope this can help you?

[alloy]

Thanks for the sleuthing @kastermester 👌

Sadly this behavior is replicated in relay itself.

Ok, so it sounds like the repro example should be updated for Flow and submitted upstream, is that correct?

[artola]

Thanks for the sleuthing @kastermester 👌

Sadly this behavior is replicated in relay itself.

Ok, so it sounds like the repro example should be updated for Flow and submitted upstream, is that correct?

@kastermester Here the execution with Flow (v5 and v6), also wrong typings too.

Am I wrong? Could you please re-check my findings?

If it is a problem, I have already (a local repo) in JS.

````
/**

• @flow
• @relayHash 0fec400190cbd557e89acaf5c3ecc19c
  */

/* eslint-disable */

'use strict';

/*::
import type { ConcreteRequest } from 'relay-runtime';
export type trialQueryVariables = {||};
export type trialQueryResponse = {|
+hero: {|
+name: string,
+__typename: "Human",
+homePlanet?: ?string,
+primaryFunction?: string,
|}
|};
export type trialQuery = {|
variables: trialQueryVariables,
response: trialQueryResponse,
|};
*/

/*
query trialQuery {
hero {
__typename
name
... on Human {
__typename
homePlanet
}
... on Droid {
__typename
primaryFunction
}
}
}
*/
```

[kastermester]

I think we should definitely compile a list of our findings in lingo/versions that the Relay team can relate to and get an issue filed upstream. The example @artola just posted is for sure something that I think anyone can perceive as a bug.

I think in the repo we should add the following examples (and their generated types):

query trialQuery {
  hero {
    name
    ... on Human {
      __typename
      homePlanet
    }
    ... on Droid {
      __typename
      primaryFunction
    }
  }
}

query trialQuery {
  hero {
    name
    __typename
    ... on Human {
      homePlanet
    }
    ... on Droid {
      primaryFunction
    }
  }
}

query trialQuery {
  hero {
    __typename
    ... on Human {
      homePlanet
      name
    }
    ... on Droid {
      primaryFunction
      name
    }
  }
}

Sadly I've just returned to work after having a cold - so I have quite a few things to catch up on. If you guys would be able to submit the bug in the Relay repo that would be great! :)

[artola]

Sadly I've just returned to work after having a cold - so I have quite a few things to catch up on. If you guys would be able to submit the bug in the Relay repo that would be great! :)

I have it ... I will open the ticket upstream.

@kastermester Thanks for the hint. It is a workaround, then I could upgrade to v5.

[artola]

@kastermester @alloy Upstream issue: https://github.com/facebook/relay/issues/2870