Generated typescript recursive query types are wrong

[freshollie]

Describe the bug

Because codegen relies heavily on Intersection types, typescript does not correctly check query fields which are optional, and allows nulls to go unchecked in code.

An example query:

type Query {
    categories(id: ID!): [Category!]!
}

type Category {
    id: ID!
    name: String!
    children: [Category!]
}

query GetCategories {
    categories(id: 1) {
        id
        name
        children {
            id
            name
            children {
                id
                name
            }
        }
    }
}

To Reproduce

Use the code sandbox:

https://codesandbox.io/s/graphql-codegen-issue-template-563vy

yarn generate will generate the query types for the nested query on Category

test.ts shows how these types provide no protection for null fields in typescript.

We generate some query response data which is a valid structure for the GetCategoriesQuery and then use that data in a function which uses the Category type as if it's children are not null (should throw an error)

yarn build will show how no type errors are generated from the types.

yarn test will show how a runtime error exists when the .length property is queried on the children field.

Expected result

GetCategoriesQuery should not be compatible with Category as children is not expected to be a null type in Category.

Environment:

• OS: MacOS
• @graphql-codegen/[email protected]:
• NodeJS: 10

Additional context

Typescript has issues with intersections being tracked microsoft/TypeScript#33243. Using them for generating these types seems to be the issue here, as manually generating the type without intersections correctly picks up the errors.

[dotansimha]

@freshollie Maybe I'm missing something, not sure.

In your use case, you are using:

children: [Category!]

Which means that children field could be null, but the array values must be set. If you wish to have a nullable type, you need to use children: [Category!]!.

With your schema, it seems to work for me in TS Playground:

And if I'm changing it to force a value there and make the field as non-nullable, it allow me to access it directly:

You shouldn't use Category type (generated by typescript) directly - it's a base type that contains all fields of the type, and then queries types can just Pick from it. GraphQL let you choose the fields and it means that always there is a base type and a selection set based on that type.

If you wish to have a dedicated type for Category of your query, use GraphQL fragments and then you'll get a type per each fragment type and it will represent the actual representation of Category based on your fragment selection set.

[freshollie]

Hey, thanks for the reply.

Firstly, the schema is correct for my requirements. I am using [Category!] as a Category may not have children, but when it does the children will not be null values.

Secondly, in my playground I am using the generated schema types directly, only to imitate the datatypes which would have come out of useGetCategoriesQuery etc. Notice I do not use the values of the data directly, as you have shown in your playground, but pass them to a function which uses my definition of Category:

import { GetCategoriesQuery } from "./types";

// This is the Category object I want to use with the data
// Notice that it doesn't accept null as a children value
interface Category {
  id: string;
  name: string;
  children?: Category[];
}

const printChildren = (children?: Category[]) => {
  if (children !== undefined) {
    console.log(children.length);
  }
};

// This function accepts only the Category type, so it
// shouldn't allow objects with nullable `children` to be provided
const handleCategories = (categories: Category[]) => {
  categories.forEach(category => printChildren(category.children));
};

const result: GetCategoriesQuery = {
  categories: [
    {
      id: "id",
      name: "name",
      children: null
    }
  ]
};

// No error when nullable children are provided
handleCategories(result.categories);

[freshollie]

Another example which shows the issue with using intersection types, which I think highlights both the issue with Typescript and my issue with this library.

type Category = {
  id: string;
  children?: Category[];
};

type Maybe<T> = T | null;

type FetchedCategory = { id: string } & {
  children: Maybe<Array<FetchedCategory>>;
};

const a: FetchedCategory[] = [
  {
    id: "a",
    children: null,
  },
];

// No error, because what?
const b: Category[] = a;

// Error, children cannot be null
const c: Category[] = [
  {
    id: "a",
    children: null,
  },
];

[n1ru4l]

I think a code sandbox reproduction would be helpful. Please also include your typescript config.

[freshollie]

My original comment:

Use the code sandbox:

https://codesandbox.io/s/graphql-codegen-issue-template-563vy

yarn generate will generate the query types for the nested query on Category

test.ts shows how these types provide no protection for null fields in typescript.

We generate some query response data which is a valid structure for the GetCategoriesQuery and then use that data in a function which uses the Category type as if it's children are not null (should throw an error)

yarn build will show how no type errors are generated from the types.

yarn test will show how a runtime error exists when the .length property is queried on the children field.

Expected result

GetCategoriesQuery should not be compatible with Category as children is not expected to be a null type in Category.

[n1ru4l]

@freshollie Sorry!

type Category = {
  id: string;
-  children?: Category[];
+  children: Category[] | null;
};

https://www.typescriptlang.org/play/index.html#code/C4TwDgpgBAwghsCBzA9gJxFAvFA3gKCigEsATALigGdg1iA7JAbkKgGMALYgG1LQnqV4iVBgDaAXSgAfKPQCu3biwC+LfKEhQAsnBAAjCAB4AKgD5sUEzLmLl+DeGgAxCME4RSw5Okw5cJBTUtAxIUCpQAGR4rJw8fAKUugbGAIJoaHpGru4cnt6iIGZmqupsKPQ0UHCUOR5eCD7iUjhirARERGSUAERwPQA0rERxvPyCtkpDRCpDEuoA9AtQAHIoUBAZ6ANQhmxw8lTQAO4cCAD8+OWVwLtCjYWSlnCLywCiW2g7own07HD0egoW6GSbcK4VKpse4iXxPVrtYaBXr9aadH7jSgKKasWb4eZAA

I am having a hard time understanding your issue.

Does this solve your issue?

https://codesandbox.io/s/graphql-codegen-issue-template-ldmhy?fontsize=14&module=%2Ftest.ts

GraphQL is AFAI not meant for recursion (this might be related: graphql/graphql-spec#91)

[freshollie]

@n1ru4l notice the first assignment b = a in my example is valid, even though the data in c is exactly the same. Typescript doesn't highlight b = a as bad because it sees the intersection type in a wrongly.

Yes, I followed the examples in graphql/graphql-spec#91 to generate my query.

Your changes to the test.ts file do not solve the issue. Run test.ts with yarn test and see how even though typescript highlighted no issues with the file there is a runtime null exception.

[freshollie]

I will point out what I am trying to show with the example source with typescript intersections:

const a: FetchedCategory[] = [
  {
    id: "a",
    children: null,
  },
];

// No error, because what? (THIS IS WRONG THERE SHOULD BE AN ERROR HERE)
const b: Category[] = a;

// Error, children cannot be null (THIS IS CORRECT, NULL IS NOT ASSIGNABLE TO CHILDREN)
const c: Category[] = [
  {
    id: "a",
    children: null,
  },
];

[n1ru4l]

Your changes to the test.ts file do not solve the issue. Run test.ts with yarn test and see how even though typescript highlighted no issues with the file there is a runtime null exception.

sandbox@sse-sandbox-ldmhy:/sandbox$ yarn test
yarn run v1.16.0
$ ts-node test.ts
0
Done in 3.72s.

[freshollie]

Oops, didn't see your logic change.

const printChildren = <T extends {}>(children: T[]) => {
  if (children) {
    console.log(children.length);
  }
};

The original logic didn't take into account nulls, because my Category interface shouldn't accept them.

const printChildren = (children?: Category[]) => {
  if (children !== undefined) {
    console.log(children.length);
  }
};

Yes, you are correct this solves the issue with the code, but is not the point in typescript and codegen to highlight issues with code before they show at runtime?

Yes, I could fix my code by changing children?: Category[] to children?: Category[] | null but this isn't the point.

The point is that I had to find this by mistake by accident, my types should have picked this up when assigning the output of the graphql query to my funciton which uses Category[]

[n1ru4l]

What is the point of redeclaring types you already generated?

What exactly could graphql-codegen change to solve your problem?

[freshollie]

Ignore the crudeness of the example I provide. The idea is that obviously I might want to use the GraphQL query data in a function which has an interface expecting a type. For example a React Component.

In this case, I should transform the data from the query response to remove the null types as the interface doesn't accept them. However, this was not highlighted. The point in the graphql types is it is supposed to highlight these issues and require the developers to make the correct data transformations in order to conform to the interface.

In the examples I have given this is not happening, due to the way codegen generates the types.

[n1ru4l]

The idea of GraphQL is to generate a schema that is consumed by the client without the need for transformation. Ideally, your Schema should represent the UI structure of your application. Of course, I can understand that this is not always possible. People use fragments for declaring data dependencies for components. E.g. Relay is 100% based on using fragments and apollo v3 will also shift towards the tendency of encouraging the usage of fragments.

In the examples I have given this is not happening, due to the way codegen generates the types.

Sorry for repeating my question: What exactly could graphql-codegen change to solve your problem?

[freshollie]

The type generated for the query is:

export type GetCategoriesQuery = (
  { __typename?: 'Query' }
  & { categories: Array<(
    { __typename?: 'Category' }
    & Pick<Category, 'id' | 'name'>
    & { children: Maybe<Array<(
      { __typename?: 'Category' }
      & Pick<Category, 'id' | 'name'>
      & { children: Maybe<Array<(
        { __typename?: 'Category' }
        & Pick<Category, 'id' | 'name'>
      )>> }
    )>> }
  )> }
);

Which should be equal to

export type FixedGetCategoriesQuery = {
  __typename?: "Query";
  categories: Array<{
    id: string;
    name: string;
    __typename?: "Category";
    children?: Maybe<
      Array<{
        id: string;
        name: string;
        __typename?: "Category";
        children?: Maybe<
          Array<{
            id: string;
            name: string;
            __typename?: "Category";
          }>
        >;
      }>
    >;
  }>;
};

When using this fixed query, typescript correctly highlights the type issues:

import { FixedGetCategoriesQuery } from "./types";

interface Category {
  id: string;
  name: string;
  children?: Category[];
}

const printChildren = (children?: Category[]) => {
  if (children !== undefined) {
    console.log(children.length);
  }
};

const handleCategories = (categories: Category[]) => {
  categories.forEach(category => printChildren(category.children));
};

const result: FixedGetCategoriesQuery = {
  categories: [
    {
      id: "id",
      name: "name",
      children: null
    }
  ]
};

handleCategories(result.categories);

Argument of type '{ id: string; name: string; __typename?: "Category" | undefined; children?: { id: string; name: string; __typename?: "Category" | undefined; children?: { id: string; name: string; __typename?: "Category" | undefined; children?: { ...; }[] | ... 1 more ... | undefined; }[] | null | undefined; }[] | null | undefined; ...' is not assignable to parameter of type 'Category[]'.
  Type '{ id: string; name: string; __typename?: "Category" | undefined; children?: { id: string; name: string; __typename?: "Category" | undefined; children?: { id: string; name: string; __typename?: "Category" | undefined; children?: { ...; }[] | ... 1 more ... | undefined; }[] | null | undefined; }[] | null | undefined; }' is not assignable to type 'Category'.
    Types of property 'children' are incompatible.
      Type '{ id: string; name: string; __typename?: "Category" | undefined; children?: { id: string; name: string; __typename?: "Category" | undefined; children?: { id: string; name: string; __typename?: "Category" | undefined; }[] | null | undefined; }[] | null | undefined; }[] | null | undefined' is not assignable to type 'Category[] | undefined'.
        Type 'null' is not assignable to type 'Category[] | undefined'.ts(2345)

[freshollie]

Does this makes sense? I understand Pick is useful for being able to use the base type, but using intersection in typescript does not behave as expected.

Codegen should generate query types without intersections in order to have correct types.

[n1ru4l]

@freshollie Does this option solve your issues? https://graphql-code-generator.com/docs/plugins/typescript-operations#preresolvetypes-boolean-default-value-false

[freshollie]

@n1ru4l yes! This creates the correct types for the query!

Do you see the issue though? By default this query is not type checked correctly, so why even generate it?

[n1ru4l]

@freshollie Shouldn't this be fixed once typescript 3.7 is out? (microsoft/TypeScript#33243)

[freshollie]

I tested with typescript@next(3.7) and it didn't fix the issue

[n1ru4l]

@freshollie I think it will take a while until it is done... (https://github.com/microsoft/TypeScript/milestone/98)

@dotansimha How do you feel about this?

[freshollie]

Yeah, advice I got was it's not 100% likely this issue gets fixed with 3.7 as the goals of the milestone change. Why is the generation with Pick the default if it doesn't work as intended?

[n1ru4l]

Why is the generation with Pick the default if it doesn't work as intended?

Because you are the first person that experienced this issue and you reported it today 😉

[freshollie]

Oh, I didn't mean to be so abrupt. I meant to mean, why does the option exist :P

[freshollie]

@dotansimha @n1ru4l any conclusions on this? To summaries: preResolveTypes: true fixes our issue, so I am fine to close.

Other people may encounter this and preResolveTypes: true is not enabled by default, so documenting this behaviour might be the solution?

[n1ru4l]

I still think this is an issue with typescript that should be documented.

[dotansimha]

@freshollie Thanks, closing for now. The current default is using Pick but I think maybe we can change it in a future version, we just need to make sure preResolveTypes is stable and has no issues.