dotansimha / graphql-code-generator

A tool for generating code based on a GraphQL schema and GraphQL operations (query/mutation/subscription), with flexible support for custom plugins.
https://the-guild.dev/graphql/codegen/
MIT License
10.83k stars 1.33k forks source link

Suggestion: Use Pick<T, K> to generate query types #713

Closed fetis closed 5 years ago

fetis commented 5 years ago

Is your feature request related to a problem? Please describe. Currently, we just have a copy of original type for a query

Describe the solution you'd like I think a more clear approach would be to use Pick<T, K> operator to make a connection to the original type.

So this would be like this

export interface TypeA {
  id: string;
  name: string;
  age: number;
}

export namespace MyQuery {
  export type TypeA = Pick<TypeA, 'id' | 'name'>
}

See it's in action here https://medium.com/@curtistatewilkinson/typescript-2-1-and-the-power-of-pick-ff433f1e6fb

ardatan commented 5 years ago

Thank you for your suggestion! That looks a good idea, but how about nested types.

query SomeQuery {
   foo
   bar
   something {
               x {
                      a
                      b
                  }
               y
    }
}
fetis commented 5 years ago

@ardatan I think it should work the same Pick<SomeQuery, 'foo' | 'something'>

fetis commented 5 years ago

here's the example with nested complex subtype in Typescript playground

the problem is that you can't reference the outer type in namespace (or I don't know how). So I've introduced alias _TypeA for server type to bypass this.

ardatan commented 5 years ago

Oh I just understand what you mean! Yes, this will reduce the code size and everything more consistent! I like that idea, and it will also solve the duplicate issues! @dotansimha What do you think? If it is ok, I can work on it!

dotansimha commented 5 years ago

It's actually looks good and more clean. I think we might need a lot of effort to support this use case, or I'm missing something? @ardatan @fetis

fetis commented 5 years ago

@dotansimha hi, sorry for a long reply. I don't think it should be more efforts than now. Instead of generating a new type every time you'll use a Pick statement on existing server type. There might some edge cases of course, but I can't forsee it now.

dotansimha commented 5 years ago

@fetis There are other things to consider, such as fragment, aliasing and more. I'm doing some research now to see if how that effects the template. In general, I agree that this syntax is way better than creating inner types.

dotansimha commented 5 years ago

@fetis we implemented this concept in the new flow plugins: https://github.com/dotansimha/graphql-code-generator/pull/917 Maybe later we'll refactor the typescript packages to use the same concept :)

itscarlosrufo commented 5 years ago

Picking this, @dotansimha where could I ping you up, have some questions about how to approach it!

itscarlosrufo commented 5 years ago

Plugin typescript-client

I've been taking a look to this feature proposal, the idea of using Pick it's quite interesting as y'all have mentioned before. I'd love to discuss what would be the best way implement this, as I have no much idea about the library, I'd really appreciate your opinion & hopefully guidance. I've noticed that the typescript-client is the responsible for generating the types that'd change, refactoring its handlebar template it'd probably work but yep, surely I'm missing a lot of corner cases.

Check this example out and let's discuss how we could solve it, the proposal is tested & working with typescript-react-apollo with no changes in its template.

Query

query allRockets {
  rockets {
    id
    name
    first_stage {
      reusable
      engines
    }
  }
}

Current Types

export type AllRocketsVariables = {};

export type AllRocketsQuery = {
  __typename?: 'Query';
  rockets: Maybe<AllRocketsRockets[]>;
};

export type AllRocketsRockets = {
  __typename?: 'Rocket';
  id: Maybe<string>;
  name: Maybe<string>;
  first_stage: Maybe<AllRocketsFirstStage>;
};

export type AllRocketsFirstStage = {
  __typename?: 'RocketFirstStage';
  reusable: Maybe<boolean>;
  engines: Maybe<number>;
};

Proposal Iterate over the fields until all of them are primitive types (didn't code it yet but I guess this is possible), it'd require to include the typescript-server in order to generate & pick the interfaces! Probably the Pick types have to include Maybe types too.

export type AllRocketsVariables = {};

export type AllRocketsQuery = {
  __typename?: 'Query';

  rockets: Maybe<AllRocketsRockets[]>;
};

export type AllRocketsRockets = Pick<Rocket, 'id' | 'name' | 'first_stage'> & {
  first_stage: Pick<RocketFirstStage, 'reusable' | 'engines'>;
};

cc/ @dotansimha @ardatan

Help wanted hahah 🙏🏻

dotansimha commented 5 years ago

@swcarlosrj that's awesome!!! we would love to get some help on this one!

So basically, today we are generating interfaces and then use them, and instead of doing that, we can use single type/interface that has all other fields nested. When we first started working on this one, TypeScript did not allow you to access a nested type in a form of MyType['fieldName'] and get the actual interface for it. Now we can do it, and we can do type aliasing so it makes it easier.

So today we have these packages:

Basically, what we should have with Pick is:

And given the following schema:

type Query {
   user(id: ID!): User
}

type User {
  id: ID!
  name: String
  email: String
}

The typescript will generate only the types, as-is (same as we do today):

export interface User {
  id: string;
  name?: string;
  email?: string;
}

And given a query that uses Query.user, the typescript-documents should use the types from typescript and only Pick the fields according to the query selection set:

query userById {
   user(id: "2") {
      id
      name
   }
}

and the output should be:

export interface UserByIdQuery {
   user: Pick<User, 'id', 'name'>
}

The exact same implementation exists in flow templates here: https://github.com/dotansimha/graphql-code-generator/tree/master/packages/plugins/flow https://github.com/dotansimha/graphql-code-generator/tree/master/packages/plugins/flow-documents

I think we can share almost the same logic with the flow plugin, just need to make sure that we are keeping support for all existing features of typescript. We also need to update typescript-resolvers accordingly.

There are some issues I noticed with Pick approach (and I addressed them in the flow plugins):

I chose to implement a partial visitor pattern solution in the flow plugin because it makes it easier to write recursive and nested code that generated output, along with some helpers that helps generate wrapping braces and more. I think we can reuse all of those from the flow plugin.

I recommend to use tools like https://www.typescriptlang.org/play/ to check the validity of the output and to verify that the objects are matching the interfaces/types.

itscarlosrufo commented 5 years ago

@dotansimha just checking this out! Thank so much for all this info, I really appreciate it! I'll give a look tomorrow, I'm pretty sure that it'll help a lot in order to design an efficient solution covering all the use cases!

NickClark commented 5 years ago

This is a huge improvement! Can't wait to see it come through! Does this also replace Maybe types with optionals? In code bases like ours, nulls are a bad practice.

NickClark commented 5 years ago

Also, it might be nice to consider exposing fragment pick types per #1312

dotansimha commented 5 years ago

WIP here: https://github.com/dotansimha/graphql-code-generator/pull/1353 :) @NickClark in the new implementation, we are using both optionals and Maybe, but you can disabled optionals with avoidOptionals and you'll be able to override the definition of Maybe (the default is Maybe<T> = T | null , but you'll be able to set it to Maybe<T> = T).

NickClark commented 5 years ago

@dotansimha Thanks! In the Typescript world, nulls are typically bad practice... but GraphQL libs tend to return nulls, so maybe there's nothing I can/should do about it. Unless there was a way for libraries like Apollo to do the conversion for us. Anyways, thanks again! Really excited to see this coming!

fetis commented 5 years ago

In the Typescript world, nulls are typically bad practice...

@NickBolles do you have a proof on that? In connection with GraphQL, I don't think we can get rid of null

NickClark commented 5 years ago

@fetis I assume you meant me. Yes. A lot of TS even non-TS projects shy away from null. The Typescript project itself doesn't use null (TS guidelines) and even Crockford doesn't use null anymore (video) That being said... after considering it further... I don't think we can get rid of null in regard to GraphQL,

dotansimha commented 5 years ago

Thank you @fetis for a great idea. It took some time, but we are finally there. All plugins are now (1.0.0) using Pick - the generated code is much simpler and it resolved so many issues.

esamattis commented 5 years ago

Just trying out the new 1.0.0 release and I have now hard time figuring out how to access deeper field types.

Example. This type is now generated for my GraphQL query:

export type ApartmentsQuery = { __typename?: "RootQuery" } & {
    apartments: Maybe<
        { __typename?: "RootQueryToApartmentConnection" } & {
            edges: Maybe<
                Array<
                    Maybe<
                        {
                            __typename?: "RootQueryToApartmentConnectionEdge";
                        } & {
                            node: Maybe<
                                { __typename?: "Apartment" } & Pick<
                                    Apartment,
                                    "id" | "title"
                                > & {
                                        buildingTypes: Maybe<
                                            {
                                                __typename?: "ApartmentToBuildingTypeConnection";
                                            } & {
                                                nodes: Maybe<
                                                    Array<
                                                        Maybe<
                                                            {
                                                                __typename?: "BuildingType";
                                                            } & Pick<
                                                                BuildingType,
                                                                "name" | "slug"
                                                            >
                                                        >
                                                    >
                                                >;
                                            }
                                        >;
                                    }
                            >;
                        }
                    >
                >
            >;
        }
    >;
};

I'm looping through the data.apartments.edges array and inside it I want to pass node.buildingTypes to an external function. In pre 1.0 versions I could just import the buildingTypes type for the external function argument types but now that all types are inlined like this. How should I go about it?

dotansimha commented 5 years ago

There are 2 options:

  1. Use MyType['fieldA']['fieldB'] until you reach your type. You can also create aliases by using MyInnerType = MyType['fieldA']['fieldB'].
  2. Use GraphQL fragments. This way the types you need to use internally will point to your fragments, and you can use the generated fragment type.

We know it's not ideal, but eliminating namespaces was very important in order to make the output optimized, and using a single type is important because it makes sure that there are no name conflicts and mis-matches in the generated output.

@epeli

esamattis commented 5 years ago
  1. Doesn't seem to work with intersections/unions?

image

  1. Fragments help a bit it's quite awkward still. You need handle bunch of Maybes and __typenames...
esamattis commented 5 years ago

It seems that I must do this to be able to reach the type:

type ArrayType<T> = T extends Array<infer V> ? V : never;

type BuildingTypes = NonNullable<
    NonNullable<
        ArrayType<
            NonNullable<NonNullable<ApartmentsQuery["apartments"]>["edges"]>
        >
    >["node"]
>["buildingTypes"];
dotansimha commented 5 years ago

@epeli I didn't understand if that's an issue. If so, can you please open a new issue with examples?

esamattis commented 5 years ago

Will do