graphql-codegen-factories icon indicating copy to clipboard operation
graphql-codegen-factories copied to clipboard

Warn when a factory would lead to an infinite loop

Open japboy opened this issue 1 year ago • 6 comments

Let's say I have a GraphQL schema like below:

type ProductCategory {
  parent_category: ProductCategory
}

And this generates mock function like below:

export function createProductCategoryMock(props: Partial<ProductCategory> = {}): ProductCategory {
  return {
    parent_category: createProductCategoryMock({}),
    ...props,
  }
}

And this cause Maximum call stack size exceeded error. Do I just need to change the parent type different name? Any thoughts? Thanks.

japboy avatar Oct 04 '22 07:10 japboy

Is parent_category field nullable? I had impression that we get nulls for all nullable fields so I expected the function to be more like:

export function createProductCategoryMock(props: Partial<ProductCategory> = {}): ProductCategory {
  return {
    parent_category: null,
    ...props,
  }
}

ertrzyiks avatar Oct 04 '22 08:10 ertrzyiks

aw the schema should be like this:

type ProductCategory {
  parent_category: ProductCategory!
}

i would like to talk about the case if the field is non-nullable :p

japboy avatar Oct 04 '22 09:10 japboy

That's an interesting use case, not sure what would be best here as the schema defines an infinite recursion. So it's impossible to satisfy the type definition, unless perhaps through a circular dependency but that would not make much sense.

How where you intending to test this code @japboy? Having a better understanding of your expectations could help here.

What do you think @ertrzyiks? Perhaps we should raise an error/warning or offer an option to trick TypeScript (for example through some null as any as ProductCategory but it could bite users).

zhouzi avatar Oct 04 '22 12:10 zhouzi

From a quick search I see people advice against it as you can't write a query that fetches all the levels of nesting. People who actually need non-nullable recursive seem to have some limits in mind on how many levels of nesting they need.

@zhouzi It would be possible to limit the depth of the resolution in the context of operation, but not in the context of the type itself which is infinitely recursive indeed.

I'm also curious what @japboy expects the function to return in this case.

I think it would be nice to tell people that this is not supported with some error or warning indeed. Like

export function createProductCategoryMock(props: Partial<ProductCategory> = {}): ProductCategory {
  throw new Error('Impossible to generate')
}

but we should be able to generate the shape that fulfils a query/mutation expected response shape where the level of nesting is known and fixed.

ertrzyiks avatar Oct 04 '22 14:10 ertrzyiks

@zhouzi @ertrzyiks thanks for your feedback!

well my use case is actually sort of exceptional. there is a schema which has nullable fields only and i need to generate non-nullable mock functions for the fields. so i wrote a patch with patch-packages which has nullableIgnored option and found the infinite recursion issue.

and i feel like this comment is intuitive for me: https://github.com/zhouzi/graphql-codegen-factories/issues/59#issuecomment-1266609192

perhaps i can add another option like nullableIgnoredExceptions: ['ProductCategory'] for my use case.

People who actually need non-nullable recursive seem to have some limits in mind on how many levels of nesting they need.

agreed. limitation is maybe passed by variables and its not expressed with types...

japboy avatar Oct 05 '22 01:10 japboy

perhaps i can add another option like nullableIgnoredExceptions: ['ProductCategory'] for my use case.

I think that would be a great solution as it's not possible to create an object with infinite depth. You would still be able to create nested objects in your code:

createProductCategoryMock({
  parent_category: createProductCategoryMock({
    // you can repeat the process as much as you need
    // or even create a helper
  })
});

With that being said, I agree with @ertrzyiks that this plugin should better inform users in this case. I think ideally it would raise an error when generating the mocks (build time as opposed to runtime). Let's add that 👍

zhouzi avatar Oct 05 '22 08:10 zhouzi