[WIP] [Relay] Better relay provided props

[alloy]

Here’s the WIP code I had for the DT typings based on the progress I had made previously. It does a few things:

• Removes the this.props.relay prop from the external component props typing.

• Recasts data props to opaque ‘fragment reference’ types in the external component props typing. In order to do this, you have to use a GeneratedNodeMap rather than a GraphQLTaggedNode, because it allows the TS type system to inflect on the prop names. This is why there are now 4 overloads per container factory function.

The only thing I had not been able to figure out yet was a way to narrow the opaque fragment references such that you can only pass the correct opaque data to the relay container, so in this PR all opaque fragment references are equal.

@kastermester I think you said you had figured out a way to be able to do that, though?

[alloy]

cc @voxmatt

[kastermester]

My approach was a bit different as I did not use the second generic type, but created the prop types just off of the Props type.

Mind you, I had not yet made an effort to support array props of fragment references properly. The general idea was the one outlined in the issue naming the type of this.props inside the component as P below:

1. Remove potential property relay from the type P.
2. For every property type at key K with type KT:
3. If the key refType exists in properties of KT - set the transformed type of KT to `{ ' fragments': KT[' refType']:
4. Otherwise set the transformed type of KT to KT
5. Add a prop named componentRef - in my tests I couldn't figure out how to get this typed correctly, but it needs to be a callback type to the original component instance type.

This means that a type such as:

interface MyProps {
  relay: any;
  myFragmentProp: {
    ' refType': MyFragmentRef;
    ' fragments': MyReferencedFragmentRef;
  };
  myNonFragmentProp: string;
}

would become:

interface MyProps {
  myFragmentProp: {
    ' fragments': MyFragmentRef;
  };
  myNonFragmentProp: string;
}

There's two ways of making unique types (given that one is not interested in runtime semantics) that I know of:

1. Declare an empty enum
2. Declare a class with some sort of private member.

Here's a playground showing some dumb initial approach (does not handle array fragment referneces and does not add the componentRef type). It should be doable to see how this can be done however.

EDIT: Was bad at making playground links. You can hover the types in the example to see what they end up as. The ContainerProps is the most interesting one.

[alloy]

Oh yeah, great solution to finding the relay provided data props! So that removes the requirement to use a map over a tagged node 👍

What isn’t really clear to me is how we can use the enum trick to make sure the right fragment reference is passed to the right container/prop. Would the enum be declared both in the container artefact that defines it (the child component) and the user of the fragment reference (the parent component) and does TS merge them and treat them as equal?

Btw, the actual prop on the opaque fragment reference is called __fragments, so I think we should just use that rather than " fragments" and while not natural in TS I did come to like the use of the $ sign to denote these props being special.

[kastermester]

Here's how it would work in practice:

File: Container1.ts

export const Container1 = createFragmentContainer(Component1, { data: graphql`fragment Container1_data on MyType { id ...Container2_data }` } );

The generated file for the fragment would look roughly like this (following the naming convention I laid out before):
File: Container1_data.graphql.ts

import { Container2_data$ref } from './Container2_data.graphql';
export const enum Container1_data$ref;
export type Container1_data {
  ' refType': Container1_data$ref;
  ' fragments': Container2_data$ref;
}

So in short: No, the enums are defined once and required and referenced from the other files as they are used.

The way it worked in the relay-compiler last time I looked - was actually that they only supported this behavior when the haste module system was enabled. I meant to talk to you about making a change here in the relay-compiler as well that allows for this to be changed as well. In the repo i invited you to you can see my solution to this issue (there's a comment in the relay-compiler repo regarding fixing this as well).

Here's the comment in the relay repo: https://github.com/facebook/relay/blob/master/packages/relay-compiler/core/RelayFlowGenerator.js#L410

Regarding naming:

I don't think it is really worth it to worry about what the runtime type here is called - the type we're giving it here really has nothing to do with the runtime type either way - and users are not meant to use the __fragments type for anything, it is internal relay usage. I mean, we're talking about typing it up to be an empty enum - which in itself provides no meaningful type - and for good reasons, we just need to identify which fragment this type is originating from - and which fragments it references. I really don't think it benefits anyone to have these types show up in their auto completion list - the data here is not useful by any stretch of the imagination. Personally I'd think it would cause more confusion than good to include an arbitrary property here. But I can be happy either way - if we do go with a property that is not automatically filtered out, I'd probably just create a language server plugin to remove it again.

[alloy]

So in short: No, the enums are defined once and required and referenced from the other files as they are used.

The way it worked in the relay-compiler last time I looked - was actually that they only supported this behavior when the haste module system was enabled.

Ah ok, I see. Yeah that’s correct.

I assume the reason they didn’t enable that outside of haste is that it would require hardcoding the path to the other artefact and thus also would need to be regenerated when referenced files change location 🤔

I meant to talk to you about making a change here in the relay-compiler as well that allows for this to be changed as well. In the repo i invited you to you can see my solution to this issue (there's a comment in the relay-compiler repo regarding fixing this as well).

Yup, found it. So yeah, generating the path initially should be fine, but I’m slightly worried about the requirement to have to update artefacts that import types from artefacts that have changed location on disk. Do you already have any thoughts about that issue?

Naively I think it would require us to implement a name->pathname map and keep track of changes, which iirc (part of) haste does.

[alloy]

I don't think it is really worth it to worry about what the runtime type here is called - the type we're giving it here really has nothing to do with the runtime type either way - and users are not meant to use the __fragments type for anything, it is internal relay usage. I mean, we're talking about typing it up to be an empty enum - which in itself provides no meaningful type - and for good reasons, we just need to identify which fragment this type is originating from - and which fragments it references.

It being about data that the user isn’t supposed to use anyways is a very fair point, my note was mostly about “if we need to name it something, why not the name it actually has?”, which leads me to…

I really don't think it benefits anyone to have these types show up in their auto completion list - the data here is not useful by any stretch of the imagination.

Are you saying that properties prefixed with whitespace are automagically removed by IDEs (such as VS Code)? Because that’s definitely a great reason to go with that.

[kastermester]

Are you saying that properties prefixed with whitespace are automagically removed by IDEs (such as VS Code)? Because that’s definitely a great reason to go with that.

Yes. You can try it out :)

EDIT: I think it might actually be properties with whitespaces in their name. I'm not sure leading/trailing matters. Furthermore, I'd guess it is simply properties that have names that are not valid identifiers.

[kastermester]

Yup, found it. So yeah, generating the path initially should be fine, but I’m slightly worried about the requirement to have to update artefacts that import types from artefacts that have changed location on disk. Do you already have any thoughts about that issue?

Naively I think it would require us to implement a name->pathname map and keep track of changes, which iirc (part of) haste does.

Yeah I didn't think about this. We could always decide to support it only when one has decided to use the config option that puts all generated files into the same directory - in which case the import becomes straight forward. It's already an option within the code - but AFAIR there's no CLI argument for it yet.

[kastermester]

I'm having some trouble creating a type in TS that can read out the element type of an array, given that the type constraints on the generic types are not allowed.

Here's what I've been trying so far:

type ReadArrayElementType<T> = (T & { [key: number]: never })[0];

However this always reads out never. The funny thing is that this technique works just fine for regular string indices, I'm thinking there might be a TS bug hidden here somewhere. Any ideas from any of you?

[alloy]

@kastermester Can you create a simple playground example that shows the issue?

[kastermester]

See this playground.

EDIT: modified the link slightly. It stated previously that T & never = T it is T | never = T.

[kastermester]

Also see the comments made here by mr. Hejlsberg himself: microsoft/TypeScript#12215 (comment)

[alloy]

Hmm, yeah, I can’t come up with a solution offhand.

I assume this is for ‘plural’ fragment references? If so, could a workaround be that we define a type for the object boxed in the array in addition to a type for the array? E.g.

export type MyPluralFragmentElement = {
  readonly id: string
  …
}
export type MyPluralFragment = ReadonlyArray<MyPluralFragmentElement>

[alloy]

@DanielRosenwasser Do you happen to know a solution to the above question?

[kastermester]

I assume this is for ‘plural’ fragment references? If so, could a workaround be that we define a type for the object boxed in the array in addition to a type for the array? E.g.

I think that is the case yes (I'm actually not sure of which cases the flow array types are used, but it seems reasonable). We might be able to emit some "helper" types for the type that lets us transform it easier.

CC: @ahejlsberg - Is there any way to read out the array element type of an array using techniques as is linked in the playground example? And/or - is it possible that the techniques you used for the Diff, Omit and Overwrite types works differently with string indexers than they do for number indexers?

Edit: Sorry about tagging both of you, thought Daniel was from the relay team, not the typescript team.

[DanielRosenwasser]

@alloy @kastermester Shoudn't T have an array constraint?

type ReadArrayElementType<T extends Array<any>> = T[number]

That should do it.

Just as a fun fact: you could probably done it the way you have above, but instead of using never (remember, never is the bottom type, it absorbs everything to never in an intersection), you can use what is effectively the top type ({} | null | undefined).

type ReadArrayElementType<T> = (T & Array<{} | null | undefined>)[number]

I'd really recommend going with the first though.

[kastermester]

@DanielRosenwasser Yeah, using constraints it is fairly trivial to solve.

The problem at hand is thst we need to take an arbitrary object and transform it, so something like the following type:

type MyType = {
  relay: any;
  fragmentProp: {
    ' $refType': RefType;
  }
  fragmentArrayProp: Array<{
   ' $refType': RefType;
  }>;
  someOtherProp: OtherType;
}

Becomes

type MyTransformedType = {
  fragmentProp: {
    ' fragments': RefType;
  }
  fragmentArrayProp: Array<{
   ' fragments': RefType;
  }>;
  someOtherProp: OtherType;
}

The solution you posted is neat though! Any idea/explanation as to why my attempt works when reading object property types but not for array element types?

[kastermester]

As far as I can tell this is the last thing we need to get fixed until we’re feature complete at this point. With the above posted type I think my last remaining troubles are fixed. I will try to get working on creating types around the container APIs next time I get some spare time free’d up.

One area we haven’t talked about are the types for the relay prop, which changes based on the container being created and the variables needed for the query (+ maybe the query result, I’m not sure). Should we create base types for the relay prop to cover this?

[DanielRosenwasser]

I think you want conditional types (microsoft/TypeScript#21316), which are slated for TypeScript 2.8.

The solution you posted is neat though! Any idea/explanation as to why my attempt works when reading object property types but not for array element types?

The problem comes up because when T gets instantiated (for some type that actually has an index signature), you'll have (T & { [x: number]: never })[number] which simplies to T[number] & { [x: number]: never}[number] which simplifies to T[number] & never which simplifies to never. You'd end up with something similar if you used any instead of never.

The solution I recommended gives (T & { [x: number]: {} | null | undefined })[number] simplifying to T[number] & ({} | null | undefined), which for some instantiation of T will give you T[number] & {} | T[number] & null | T[number] & undefined.

[kastermester]

I think you want conditional types (microsoft/TypeScript#21316), which are slated for TypeScript 2.8.

Awesome, I was unaware that this was on the roadmap! But yes, for most of the issues I'm having here this feature would solve them! (the last thing I'd need is some type subtraction feature - but, disregarding super/sub-typing issues this is a solved thing)

The problem comes up because when T gets instantiated (for some type that actually has an index signature), you'll have (T & { [x: number]: never })[number] which simplies to T[number] & { [x: number]: never}[number] which simplifies to T[number] & never which simplifies to never. You'd end up with something similar if you used any instead of never.

The solution I recommended gives (T & { [x: number]: {} | null | undefined })[number] simplifying to T[number] & ({} | null | undefined), which for some instantiation of T will give you T[number] & {} | T[number] & null | T[number] & undefined.

This makes sense - I just don't get why I don't get the same issue with this piece of code:

type ReadProperty<T, Prop extends string> = (T & { [key: string]: never })[Prop];

Which is basicly T[P] implemented without type constraints, yielding never if T has no property P. It seems to me like string and number indexes works differently?

[kastermester]

@alloy Having read up briefly on the PR for conditional types I’m thinking our ts typings for the createXContainer methods should probably wait for that feature, it seems like a much more robust solution.

Do you have an idea of any missing parts in your PR there are to type up? (Compat/Classic APIs, the imperative mutations, subscriptions, etc?)

[alloy]

@kastermester That’s fair. So how about for now we bring this PR into a state where it will satisfy the typings emitted by our TS plugin (i.e. have relay-runtime export ConcreteFragment, ConcreteRequest) and remove the relay prop from the container props interface.

I think we can also still leave the flat single artefacts dir in place and import fragment references, but for now that would only benefit those that use @relay(mask: false), others will need to cast the reference as any before passing it to the child component, like they do now.

[alloy]

I have not spent any time and effort on Classic/Compat mode, seeing as I no longer use those. Do you wanna take a stab at that?

[kastermester]

@kastermester That’s fair. So how about for now we bring this PR into a state where it will satisfy the typings emitted by our TS plugin (i.e. have relay-runtime export ConcreteFragment, ConcreteRequest) and remove the relay prop from the container props interface.

I think we can also still leave the flat single artefacts dir in place and import fragment references, but for now that would only benefit those that use @relay(mask: false), others will need to cast the reference as any before passing it to the child component, like they do now.

Alright sounds good.

I have not spent any time and effort on Classic/Compat mode, seeing as I no longer use those. Do you wanna take a stab at that?

I will try to get that into the PR as well. Like I mentioned previously getting classic/compat mode running is quite important to us (at least the compat API should be pretty much similiar to the modern one).

I'm not sure exactly when I'll have time to look at this again, but if I'm lucky I'll have small bits and pieces here and there. Will keep you updated.

[alloy]

Like I mentioned previously getting classic/compat mode running is quite important to us (at least the compat API should be pretty much similiar to the modern one).

Yup, I totally understand 👍

I suspect the compat one should be easy. If I find time sooner than you, I’ll make the modern and compat ones in order and we can merge those first, because I’m looking to start using this all internally ASAP.

[kastermester]

I suspect the compat one should be easy. If I find time sooner than you, I’ll make the modern and compat ones in order and we can merge those first, because I’m looking to start using this all internally ASAP.

Sounds good. FYI here's the thoughts I had regarding the ConcreteFragment and friends.

In the typings, export ConcreteFragment and friends as:

export enum ConcreteFragment {}

In the generated artifacts (for fragments) generate the *_ref (or whatever we end up deciding on as far as naming goes) as:

import { ConcreteFragment } from 'react-relay';
enum MyFragment_ref_type {}
export type MyFragment_ref = MyFragment_ref_type & ConcreteFragment;

And then something similiar for the requests (artifacts for query, mutations and subscriptions).

Then when you need to create a function that takes some kind of fragment you can type it up as (again depending on chosen naming convention):

interface FragmentRef {
  ' refType': ConcreteFragment;
}
export function myFunctionThatNeedsAFragment(fragment: FragmentRef): void;

[kastermester]

Also: Note that the above stated types are not actually really checked anywhere (ie. it is the return type of the graphql function that is important, it should probably be something like { 'refType': ConcreteFragment & ConcreteRequest }).

Come to think of it, I'm not really sure if modelling and typing this up is that important. Just ensuring some unique type - which is then referenced by all the places where the return type of that function is expected.

[voxmatt]

Hey all, I've been silently watching this; great work. Apologies for not being a help here; work has been incredibly busy of late.

[kastermester]

No worries @voxmatt :) I think we all know how work can be sometimes :D

[alloy]

@voxmatt Never need to be sorry for not being able to contribute to OSS!

[alloy]

@kastermester Btw, why did you opt for type instead of interface for the fragment selections?

[kastermester]

No particular reason - going with interfaces sounds good :)

[alloy]

This is what I’m currently writing up as tests for this PR to be examples of artefacts:

enum _TodoItem_item$ref {}
export type TodoItem_item$ref = _TodoItem_item$ref & ConcreteFragment;
interface TodoItem_item {
    readonly text: string;
    readonly isComplete: boolean;
    readonly " $refType": TodoItem_item$ref;
}

enum _FeedStories_feed$ref {}
type FeedStories_feed$ref = _FeedStories_feed$ref & ConcreteFragment;
interface FeedStories_feed {
    readonly stories: {
        readonly edges: ReadonlyArray<{
            readonly node: {
                readonly id: string;
                readonly " $fragmentRefs": Story_story$ref;
            };
        }>;
    };
    readonly " $refType": FeedStories_feed$ref;
}

I chose to prefix the unique enum with an underscore, because it’s really just a part of the exported declaration but because it’s an enum it needs a unique name.

[kastermester]

@alloy seems good! (assuming you're exporting the interface and the non underscored ref type!

I noticed this PR is now up in the typscript repo as well: microsoft/TypeScript#21496

This should allow us to type up the componentRef prop as well, once TS 2.8 lands. (assuming that PR goes into 2.8).

[kastermester]

Oh and btw, we should probably look into making the enums const, that way they leave no artifacts when being compiled to JavaScript.

[kastermester]

Using the code in that PR I have been able to try out the following:

import { ConcreteFragment } from './ConcreteFragment';
type Diff<T extends string, U extends string> = ({[P in T]: P } & {[P in U]: never } & { [x: string]: never })[T];
type Omit<T, K extends keyof T> = Pick<T, Diff<keyof T, K>>;
type InstanceType<T> = T extends new (...args: any[]) => infer U ? U : never;


export type FragmentOrRegularProp<T> = T extends { ' $refType': infer U} ? { ' $fragmentRefs': U } : T;

type NoRelay<T> = Omit<T & { 'relay'?: any }, 'relay'>;

export type MappedFragmentProps<T> = {
	[K in keyof T]: FragmentOrRegularProp<T[K]>;
}

export type ContainerProps<Props, TContainerConstructor> = MappedFragmentProps<NoRelay<Props>> & {
	componentRef?: (ref: InstanceType<TContainerConstructor> | null) => void
};

export type ConstructorProps<T> = T extends React.ComponentClass<infer P> ? P : never;

export declare function createFragmentContainer<TContainer extends React.ComponentClass<any>>(
	Component: TContainer,
	fragmentSpec: any
): React.ComponentClass<ContainerProps<ConstructorProps<TContainer>, TContainer>>;

(Note, I still don't handle array fragments like the flow code, but now it should be more than possible). Also I only deal with components derived from React.Component, but I suspect that should be easy to update.

This properly types the componentRef property. I think we should be able to make an "all magic" approach using this - once TS 2.8 lands.

Great work on the typings so far @alloy :) Looking good.

[alloy]

Going to close this in favour of DefinitelyTyped#23460. We have a ticket to keep track of all the work we need to do which is discussed here https://github.com/kastermester/relay-compiler-language-typescript/issues/7.