Skip to content
This repository has been archived by the owner on Sep 27, 2023. It is now read-only.

Emit separate interfaces for each type and its selections #64

Closed
alloy opened this issue Aug 29, 2018 · 28 comments
Closed

Emit separate interfaces for each type and its selections #64

alloy opened this issue Aug 29, 2018 · 28 comments

Comments

@alloy
Copy link
Member

alloy commented Aug 29, 2018

Often we need to have a reference to a type that’s being used somewhere deeper into a fragment’s query. For instance, consider this fragment’s typings:

export type ArtworkSidebarArtists_artwork = {
    readonly artists: ReadonlyArray<({
        readonly name: string | null;
        readonly href: string | null;
    }) | null> | null;
};

To some functions we end up passing a single element of the artists array, so currently in our code we do the following:

type Artist = ArtworkSidebarArtists_artwork["artists"][0]

It would be easy to emit the following artefact instead:

export type Artist = {
    readonly name: string | null;
    readonly href: string | null;
};

export type ArtworkSidebarArtists_artwork = {
    readonly artists: ReadonlyArray<Artist | null> | null;
};

The only situation I imagine where it gets more complex is around naming the types when you have multiple selections on the same type in a single fragment. For instance, let’s say we had the following fragment typing:

export type ArtworkSidebarArtists_artwork = {
    readonly artists: ReadonlyArray<({
        readonly name: string | null;
        readonly related_artists: ReadonlyArray<({
            href: string | null;
        }) | null> | null;
    }) | null> | null;
};

In this case we can’t just call the type Artist, because we have to differentiate between the artwork’s artist and the artists related to the artwork’s artist.

Encoding the path into the name of the type (e.g. ArtworkSidebarArtists_artwork_artists_related_artists_Artist) would be counter-productive, as in that case you could just as well use plain TS code to get the type reference (e.g. ArtworkSidebarArtists_artwork["artists"][0]["related_artists"][0]).

Maybe this is enough of an edge-case that we simply don’t emit named typings for such situations at all, which could instead encourage people to refactor that nested part of the query into a container/fragment of its own?

@alloy
Copy link
Member Author

alloy commented Aug 29, 2018

An even more compelling argument could be made for unions, as there’s no way (that I know of?) to get a reference to a single type in a union with just TS.

@kastermester
Copy link
Contributor

kastermester commented Aug 29, 2018

We have run into the same issue at our (still sigh) legacy based solution. The issue is that there's really not a great way to name these types.

I think the intent behind relay is to have a named fragment (not used directly within a container) and then spread it using @relay(mask: false) define it using @relay(mask: false). But perhaps this is an issue we should take up directly with the relay team?

See: https://facebook.github.io/relay/docs/en/graphql-in-relay.html#relaymask-boolean

@kastermester
Copy link
Contributor

FYI we solve this, somewhat, in our old solution using directives inside the GraphQL fragment type, ie.

fragment on MyType {
  name
  profile @generateType(name: "Profile") {
     id
     name
  }
}

This still does not solve the union issue though, and it is also a bit complicated when the given type is a list type.

@ds300
Copy link
Contributor

ds300 commented Aug 29, 2018

@alloy
Copy link
Member Author

alloy commented Aug 29, 2018

@ds300 Ooh nice one, didn’t know that one exist yet! And it’s so trivial in retrospect: type Extract<T, U> = T extends U ? T : never;

@alloy
Copy link
Member Author

alloy commented Aug 29, 2018

@kastermester Oh interesting thought! Yeah, that technically works.

Although I do think that means we need to be careful that the processed AST from relay-compiler doesn’t actually get required at runtime and added to the JS bundle generated by e.g. webpack. An easy fix would be for the fragment to exist in a separate module that never actually gets required anywhere, but I don’t like how that breaks the co-location aspect 🤔


Currently, however, this is breaking on our emitted fragment reference type checking. Consider the following example:

export const ShippingAndPaymentReviewFragmentContainer = createFragmentContainer(
  ShippingAndPaymentReview,
  graphql`
    fragment ShippingAndPaymentReview_order on Order {
      fulfillmentType
      shippingName
      shippingAddressLine1
      shippingAddressLine2
      shippingCity
      shippingPostalCode
      shippingRegion
      lineItems {
        edges {
          node {
            artwork {
              shippingOrigin
            }
          }
        }
      }
      creditCard {
        brand
        last_digits
        expiration_year
        expiration_month
      }
    }
  `
)

...which gets split up into:

graphql`
  fragment ShippingAndPaymentReview_orderAddressDetails on Order {
    shippingName
    shippingAddressLine1
    shippingAddressLine2
    shippingCity
    shippingPostalCode
    shippingRegion
  }
`

export const ShippingAndPaymentReviewFragmentContainer = createFragmentContainer(
  ShippingAndPaymentReview,
  graphql`
    fragment ShippingAndPaymentReview_order on Order {
      fulfillmentType
      ...ShippingAndPaymentReview_orderAddressDetails @relay(mask: false)
      lineItems {
        edges {
          node {
            artwork {
              shippingOrigin
            }
          }
        }
      }
      creditCard {
        brand
        last_digits
        expiration_year
        expiration_month
      }
    }
  `
)

In this case the emitted typings look like:

export type ShippingAndPaymentReview_orderAddressDetails = {
    // ...
    readonly " $refType": ShippingAndPaymentReview_orderAddressDetails$ref;
};

export type ShippingAndPaymentReview_order = {
    readonly fulfillmentType: OrderFulfillmentType | null;
    readonly shippingName: string | null;
    // ...
};

For the data to be assignable to the address details type, the parent should include " $fragmentRefs": ShippingAndPaymentReview_orderAddressDetails$ref regardless of its data not being masked.

@alloy
Copy link
Member Author

alloy commented Aug 29, 2018

@kastermester The custom directive is an interesting alternative and simplifies some of these things, just feels a bit less clean to mix such details into the fragment. Although OTOH @relay(mask: false) in this example also doesn’t nicely convey the semantics of its usage 🤔

So yeah, might be something to talk to the Relay team about and also check if the emitted Flow types do work in the @relay(mask: false) scenario.

@kastermester
Copy link
Contributor

Without having tried this, does the type checking work if you move the directive to the fragment itself (not when it is being spread) ie.

graphql`fragment ShippingAndPaymentReview_orderAddressDetails on Order @relay(mask: false) {
    shippingName
    shippingAddressLine1
    shippingAddressLine2
    shippingCity
    shippingPostalCode
    shippingRegion
  }

It was my understanding that this should work (from reading the docs) but I have not yet tried this myself.

@kastermester
Copy link
Contributor

Oh also - the fragment ref is, the way I understand it, not supposed to be included in the fragment refs. The idea is not to use the @relay(mask: false) when you want to render a component that requires some data - it is a way of embedding a fragment inside your component while you want access to the data only inside that component. I think if you want both (access to the data + rendering another component with that same data) you should do one of the following:

  1. Define two different fragments - possibly requiring the same data, and define one using @relay(mask: false) and one without. Use the non-masked one inside the parent component and the other one inside the child component.
  2. Define only one and spread it twice, once with mask false and once without the directive. I don't think this one plays as nicely for most use cases however. One should be really careful using the directive for this.

Directives with @relay(mask: false) to me really reads "copy&paste the fragment data in here into the query". Ie. you should not see it as "I want to render another component using its fragment" but rather "for some reason I do not want to type out the data I need here, so I'm just referring to it using this fragment name". In this case that reason is simply that you want a name for that piece of data.

@kastermester
Copy link
Contributor

kastermester commented Aug 29, 2018

One last thing:

One thing that should work however is that if we have the following fragments:

fragment MyComponent_nonMaskedFragment on SomeType @relay(mask: false) {
   id
   otherType {
     ...OtherComponent_prop
   }
}

fragment MyComponent_prop on SomeType {
  ...MyComponent_nonMaskedFragment
  id
}

fragment OtherComponent_prop on SomeOtherType {
  id
}

Obviously using this - we need to be able to render OtherComponent using the data from MyComponent_prop. We probably need some tests around this (if they are not already there somehow from the tests we copied from the official repo). I would expect the type to be something like:

export type MyComponent_nonMaskedFragment = {
    // Not sure if this will be here or not:
    readonly " $refType": MyComponent_nonMaskedFragment$ref;
    // ...
    readonly id: string;
    readonly otherType: {
      readonly " $fragmentRefs": OtherComponent_prop$ref;
    };
    // ...
};

export type MyComponent_prop = {
    readonly " $refType": MyComponent_Prop$ref;
    // ...
    readonly id: string;
    readonly otherType: {
      readonly " $fragmentRefs": OtherComponent_prop$ref;
    }
    // ...
};

Does this make sense?

@artola
Copy link
Contributor

artola commented Jan 10, 2019

@alloy @sibelius could you please refresh the status of this issue? As you stated at the beginning, separate types for each interface is something more than nice to have.
In my case, trying to get the types of a deep leaf, is a no go type X = mybag[0]['transactions']['edges'][0]['node']; ... on top, some are nullables.

@tomconroy
Copy link

Hello! We have a strong use-case for this kinda thing, especially because our schema is mostly nullable types, and we have strictNullChecks. So to get to anything reasonably deep, it's necessary to do stuff like this:

type RecircNode = NonNullable<
  NonNullable<
    NonNullable<
      NonNullable<
        NonNullable<VideoPlayerNextUpData_viewer['realmContent']>['recircForNode']
      >['edges']
    >[0]
  >['node']
>;

type RecircSeriesMember = NonNullable<
  NonNullable<NonNullable<RecircNode['members']>['edges']>[0]
>['node'];

It would be lovely to be able to add something like @generateType(name: "RecircNode") and get these types for free

@artola
Copy link
Contributor

artola commented Jan 29, 2019

@tomconroy I am with you, it would be nice to get the types split in parts (apollo's way).

In the meantime, just a hacky trick for similar case than yours, create a fragment of the node, and then spread it in the position needed. In this way I get the typings for that fragment.

e.g.

// issues_node.ts
import {graphql} from 'react-relay';

// tslint:disable-next-line: no-unused-expression
graphql`
  fragment issuesNode on Issue @relay(mask: false) {
    id
    title
    repository {
      name
    }
    viewerDidAuthor
    state
  }
`;

==> generates

// issuesNode.graphql.ts
import { ConcreteFragment } from "relay-runtime";
export type IssueState = "CLOSED" | "OPEN" | "%future added value";
declare const _issuesNode$ref: unique symbol;
export type issuesNode$ref = typeof _issuesNode$ref;
export type issuesNode = {
    readonly id: string;
    readonly title: string;
    readonly repository: {
        readonly name: string;
    };
    readonly viewerDidAuthor: boolean;
    readonly state: IssueState;
    readonly " $refType": issuesNode$ref;
};

Now we are a way closer, excepts the extra $refType, possible to remove with:

/**
 * Removes recursively all traces of `$fragmentRefs` and `$refType` from type.
 * @see https://github.com/relay-tools/relay-compiler-language-typescript/issues/29#issuecomment-417267049
 */
type NoFragmentRefs<T> = T extends object
  ? {
      [P in Exclude<keyof T, ' $fragmentRefs' | ' $refType'>]: NoFragmentRefs<
        T[P]
      >
    }
  : T;

@alloy
Copy link
Member Author

alloy commented Jan 29, 2019

I actually have come around and think that using fragments to split these up is the way it should be done, as per @kastermester's original comments.

The major problem I see with trying to add such a feature is naming. In a single fragment you could be selecting different fields of the same type at various levels in the graph, because of that you can't just name a selection set after the type, but you'd have to somehow make them unique. By using fragments you get to control at what level you actually want this separation and how to name things.

As for the fragment reference, I'm not sure what the upstream Flow emission does, but it seems to me that if a fragment is defined with the @relay(mask: false) directive there isn't any need for us to emit the opaque fragment references, so that's something that we could perhaps change to improve the experience.

@jstejada @josephsavona do you have any thoughts on this?

@josephsavona
Copy link

The major problem I see with trying to add such a feature is naming. In a single fragment you could be selecting different fields of the same type at various levels in the graph, because of that you can't just name a selection set after the type, but you'd have to somehow make them unique. By using fragments you get to control at what level you actually want this separation and how to name things.

This. I strongly agree with your conclusion for the exact reasons you listed. We highly recommend using fragments as a way to describe a unit of data that you want to access.

@alloy
Copy link
Member Author

alloy commented Jan 29, 2019

@josephsavona Thanks 🙏

@tomconroy
Copy link

@josephsavona @alloy thank you for the feedback, this makes sense. I did go down this route, trying to use @relay(mask: false) but ran into this issue: facebook/relay#2118. It's impossible to have arguments inside such a fragment? If that issue gets fixed then named fragments are a great solution

@dotbear
Copy link

dotbear commented Jul 16, 2019

So I originally found this issue because I needed to send parts of the type to a function to do work with it (similar to above), however this was successfully solved using fragments like suggested (yay).

However now I've run into the union problem - did anyone solve this properly? I'm rendering a form and showing certain fields based on what type of product is being edited, while most fields are shared between the types (showing shipping settings for a physical product, but file settings for a digital one). I'm not really sure how to solve this?

I'd preferably like to use a type guard

(product: Product): product is PhysicalProduct => product.__typename === "PhysicalProduct"

however I don't know how I would derive types Product, PhysicalProduct, and DigitalProduct here using relay-compiler-language-typescript.

Here is my example query

product(uuid: $uuid) {
  __typename
  name
  summary
  description

  ... on PhysicalProduct {
    estimatedProcessingTime
    packagingType
  }

  ... on DigitalProduct {
    files { [...] }
  }
}

Any thoughts?

@dotbear
Copy link

dotbear commented Jul 16, 2019

Hm, I suppose maybe I could @relay(mask: false) the PhysicalProduct and DigitalProduct parts, as well as the common query parts which would get me Common, PhysicalProduct, and DigitalProduct thus allowing me to define

type FullPhysicalProduct = Common & PhysicalProduct
type FullDigitalProduct = Common & DigitalProduct

Maybe?

@dotbear
Copy link

dotbear commented Jul 16, 2019

Given the following root query in my QueryRenderer:

product(uuid: $uuid) {
  ...EditProduct_product @relay(mask: false)
}

I then get

graphql`
  fragment EditProduct_product on Product @relay(mask: false) {
    ...EditProduct_common @relay(mask: false)

    ... on MerchProduct {
      ...EditProduct_merch @relay(mask: false)
    }
  }
`;

graphql`
  fragment EditProduct_merch on MerchProduct @relay(mask: false) {
    __typename
    estimatedProcessingTime
    packagingType
  }
`;

graphql`
  fragment EditProduct_common on Product @relay(mask: false) {
    __typename
    name
    summary
    description
  }
`;

export type NoRefs<T> = T extends object ? Omit<T, " $refType" | " $fragmentRefs"> : T;

export type MerchProduct = NoRefs<EditProduct_common> & NoRefs<EditProduct_merch>;

export const isMerchProduct = (product: NoRefs<EditProduct_product>): product is MerchProduct =>
  product.__typename === "MerchProduct";

Here is what I ended up doing - in case others are looking for a solution

@maraisr
Copy link
Member

maraisr commented Dec 10, 2019

Can I just interject (and probably a side question); what is the use-case for mask: false, in what use case would this be a good idea? I mean, like what stops a spread from changing fields, and then breaking upstream? I thought the whole idea was to declare data requirements for a particular thing. And if you want to read things that is masked, make a new fragment out of it, place it in a readInlineData, and be safe there also.

@josephsavona
Copy link

josephsavona commented Dec 11, 2019

@maraisr The use-case is things like utility functions that are not executing in a React context and therefore don't have access to the context's environment. @relay(mask:false) was our earlier solution to this (which as you noted has some issues), @inline is its replacement.

@josephsavona
Copy link

Agreed though - now that we have @inline we should recommend that for utility functions, and may not need the separate types for each part of a fragment.

@alloy
Copy link
Member Author

alloy commented Dec 18, 2019

Yeah, seems fair to close this by now 👍

@alloy alloy closed this as completed Dec 18, 2019
@olso
Copy link

olso commented Jan 3, 2020

Little dirty, but works for my use case

// https://github.com/relay-tools/relay-compiler-language-typescript/issues/64#issuecomment-511765083
type NoRefs<T> = T extends object ? Omit<T, " $refType" | " $fragmentRefs"> : T;

// https://www.typescriptlang.org/docs/handbook/advanced-types.html#type-inference-in-conditional-types
// https://stackoverflow.com/questions/43537520/how-do-i-extract-a-type-from-an-array-in-typescript#comment94888844_52331580
type Unpacked<T> = T extends Array<infer U> ? U : T extends ReadonlyArray<infer U> ? U : T;

type ExtractRelayEdgeNode<T> = NoRefs<
  NonNullable<NonNullable<Unpacked<NonNullable<NonNullable<T>["edges"]>>>["node"]>
>;

and use it like

type EdgeNode = ExtractRelayEdgeNode<SomeEdges_list>;
type EdgeNode = ExtractRelayEdgeNode<SomeEdges_list["search"]>;

@bitofbreeze
Copy link

@olso I get

Type '"node"' cannot be used to index type 'NonNullable<Unpacked<NonNullable<NonNullable<T>["edges"]>>>'

in typescript 3.8.3

@olso
Copy link

olso commented Sep 12, 2020

@CSFlorin

Just hit it as well, this seems to work for me with typescript 4.0.1

// https://github.com/relay-tools/relay-compiler-language-typescript/issues/64#issuecomment-511765083
export type NoRefs<T> = T extends Record<string, unknown>
  ? Omit<T, " $refType" | " $fragmentRefs">
  : T;

export type ExtractRelayEdgeNode<
  T extends { edges: ReadonlyArray<{ node: any | null }> | null } | null
> = NoRefs<NonNullable<NonNullable<NonNullable<NonNullable<T>["edges"]>[0]>["node"]>>;

@RichardLindhout
Copy link

RichardLindhout commented Apr 17, 2021

@olso solutions worked for me but I needed one extra null of the edge node

// https://github.com/relay-tools/relay-compiler-language-typescript/issues/64#issuecomment-511765083
export type NoRefs<T> = T extends Record<string, unknown>
  ? Omit<T, ' $refType' | ' $fragmentRefs'>
  : T;

export type ExtractRelayEdgeNode<
  T extends { edges: ReadonlyArray<{ node: any | null } | null> | null } | null
> = NoRefs<
  NonNullable<NonNullable<NonNullable<NonNullable<T>['edges']>[0]>['node']>
>;

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests