#9962 causes issues with our schema: parts of the types in ResolversType map contradict other parts of the type (repro)

mx-bernhard commented 3 months ago

Which packages are impacted by your issue?

@graphql-codegen/visitor-plugin-common, @graphql-codegen/typescript-resolvers

Describe the bug

With the update of @graphql-codegen/visitor-plugin-common to 5.2.0 and from that very likely this PR we run into an issue with the generated types. To me, it looks like the resolver types are improved, by replacing the base interface with its concrete implementations. This does not seem to be generated correctly in all cases.

Take the following graphql.schema (also in the Github repro repo):

type Assignment {
  resource: HumanResource
}

interface Resource {
  id: ID!
  attributes: [Attribute!]!
}

type VehicleResource implements Resource {
  id: ID!
  attributes: [Attribute!]!
  plateNumber: String!
}

type HumanResource implements Resource {
  id: ID!
  attributes: [Attribute!]!
  fullName: String!
}

interface Attribute {
  id: ID!
}

type JSONAttribute implements Attribute {
  id: ID!
  json: String!
}

type TextAttribute implements Attribute {
  id: ID!
  text: String!
}

type Query {
  assignment: Assignment!
  humanResource: HumanResource!
  resource: Resource!
  attribute: Attribute!
}

It contains two levels of interface to type mappings. When you look at the type generated for the Assignment resolver, the resource points to the schema base type instead of the resolver type:

export type ResolversTypes = {

  // has resource: HumanResource instead of resource: ResolversTypes<'HumanResource'>
  Assignment: ResolverTypeWrapper<Assignment>; // !!!

  Attribute: ResolverTypeWrapper<ResolversInterfaceTypes<ResolversTypes>['Attribute']>;
  Boolean: ResolverTypeWrapper<Scalars['Boolean']['output']>;
  HumanResource: ResolverTypeWrapper<Omit<HumanResource, 'attributes'> & { attributes: Array<ResolversTypes['Attribute']> }>;
  ID: ResolverTypeWrapper<Scalars['ID']['output']>;
  JSONAttribute: ResolverTypeWrapper<JsonAttribute>;
  Query: ResolverTypeWrapper<{}>;
  Resource: ResolverTypeWrapper<ResolversInterfaceTypes<ResolversTypes>['Resource']>;
  String: ResolverTypeWrapper<Scalars['String']['output']>;
  TextAttribute: ResolverTypeWrapper<TextAttribute>;
  VehicleResource: ResolverTypeWrapper<Omit<VehicleResource, 'attributes'> & { attributes: Array<ResolversTypes['Attribute']> }>;
};

What it should do, is:

export type ResolversTypes = {

  // change type of resource to ResolversTypes['HumanResource']
  Assignment: ResolverTypeWrapper<Omit<Assignment, 'resource'> & { resource: ResolversTypes['HumanResource'] }>;

  Attribute: ResolverTypeWrapper<ResolversInterfaceTypes<ResolversTypes>['Attribute']>;
  Boolean: ResolverTypeWrapper<Scalars['Boolean']['output']>;
  HumanResource: ResolverTypeWrapper<Omit<HumanResource, 'attributes'> & { attributes: Array<ResolversTypes['Attribute']> }>;
  ID: ResolverTypeWrapper<Scalars['ID']['output']>;
  JSONAttribute: ResolverTypeWrapper<JsonAttribute>;
  Query: ResolverTypeWrapper<{}>;
  Resource: ResolverTypeWrapper<ResolversInterfaceTypes<ResolversTypes>['Resource']>;
  String: ResolverTypeWrapper<Scalars['String']['output']>;
  TextAttribute: ResolverTypeWrapper<TextAttribute>;
  VehicleResource: ResolverTypeWrapper<Omit<VehicleResource, 'attributes'> & { attributes: Array<ResolversTypes['Attribute']> }>;
};

Otherwise, a mixture of types from ResolversTypes and schema types clash with each other when trying to implement a resolver for assignment:

 Type 'Omit<HumanResource, "attribute"> & { attribute: ResolverTypeWrapper<JsonAttribute | TextAttribute>[]; }' is not assignable to type 'Maybe<HumanResource>'.
   Type 'Omit<HumanResource, "attribute"> & { attribute: ResolverTypeWrapper<JsonAttribute | TextAttribute>[]; }' is not assignable to type 'HumanResource'.
     Type 'Omit<HumanResource, "attribute"> & { attribute: ResolverTypeWrapper<JsonAttribute | TextAttribute>[]; }' is not assignable to type 'Resource'.
       Types of property 'attribute' are incompatible.
         Type 'ResolverTypeWrapper<JsonAttribute | TextAttribute>[]' is not assignable to type 'Attribute[]'.
           Type 'ResolverTypeWrapper<JsonAttribute | TextAttribute>' is not assignable to type 'Attribute'.
             Property 'id' is missing in type 'Promise<JsonAttribute | TextAttribute>' but required in type 'Attribute'.ts(2322)

See also this commit for how, I think, it should have been generated. To me, it looks like the logic for generation does not consider nested cases of references between interfaces that need to be resolved to their types, but I might be wrong, it's just a feeling.

Your Example Website or App

https://github.com/mx-bernhard/graphql-codegen-resolver-types

Steps to Reproduce the Bug or Issue

See repro repo. The error is visible in resolvers-types-server.ts in line 57.

I checked in the generated files. They were generated with this:

yarn install
yarn generate

Expected behavior

See this commit for how, I think, it should have been generated.

Screenshots or Videos

No response

Platform

OS: [Windows WSL 2]
NodeJS: [e.g. 20.13.1]
@graphql-codegen/cli 5.0.2
@graphql-codegen/typescript 4.0.7
@graphql-codegen/typescript-resolvers 4.1.0
graphql 16.8.1

Codegen Config File

No response

Additional context

The behavior cannot be observed with version 4.0.6 of @graphql-codegen-typescript-resolvers

eddeee888 commented 3 months ago

Hi @mx-bernhard ,

I've got an alpha version of @graphql-codegen/typescript-resolvers to see if it fixes your issue. Could you please try it?

If you are using @graphql-codegen/typescript-resolvers directly:

yarn add -DE @graphql-codegen/typescript-resolvers4.2.0-alpha-20240619113651-03ef18eeeddd5c17b03561e50b3a938e9eb0a6ad

If you use server preset, you may have to use Yarn's resolutions (or the equivalent of it if you are using other package manegers) in your package.json:

// package.json
{
  "resolutions": {
    // Note: we need both entries here:
    "@graphql-codegen/typescript-resolvers": "4.2.0-alpha-20240619113651-03ef18eeeddd5c17b03561e50b3a938e9eb0a6ad",
    "@graphql-codegen/visitor-plugin-common": "5.3.0-alpha-20240619113651-03ef18eeeddd5c17b03561e50b3a938e9eb0a6ad"
  }
}

mx-bernhard commented 3 months ago

Hi @eddeee888 , I DM'ed you via your e-mail the entire schema and a few notes. Maybe, you can create a repro from this and update this issue with the findings.

mx-bernhard commented 3 months ago

I just pushed an additional commit on the repro from above with the following remaining issues in our code base:

What was generated:

  Operation: ResolverTypeWrapper<
    Omit<Operation, "assignment"> & {
      assignment?: Maybe<ResolversTypes["Assignment"]>;
    }
  >;
  Order: ResolverTypeWrapper<Order>;

What should have been generated:

  Operation: ResolverTypeWrapper<
    Omit<Operation, "assignment" | "confirmation" | "order"> & {
      assignment?: Maybe<ResolversTypes["Assignment"]>;
      confirmation?: Maybe<ResolversTypes['Confirmation']>;
      order: ResolversTypes["Order"];
    }
  >;
  Order: ResolverTypeWrapper<
    Omit<Order, "operations"> & {
      operations: Array<ResolversTypes["Operation"]>;
    }
  >;

Order in Operation should be replaced with its ResolversTypes lookup because:

Order references Operation
Operation references Assignment
Assignment references Attribute which is an interface with two implementations

Operation in Order should be replaced with its ResolversTypes lookup because:

Operation references Assignment
Assignment references Attribute which is an interface with two implementations

Confirmation in Operation should be replaced with its ResolversTypes lookup because:

Confirmation references FlatRateConfirmation
FlatRateConfirmation references Resource which is an interface with multiple implementations

The first can be shortened to this notion: Order in Operation should be replaced with its ResolversTypes lookup because, Order references Operation which is a type which already had to be replaced. The implementation has to be recursive for it to cover all kinds of schemas.

eddeee888 commented 3 months ago

Thanks for the detailed explanation @mx-bernhard !

I've released the following alpha versions and tested on the provided schema:

@graphql-codegen/visitor-plugin-common@5.3.0-alpha-20240625114132-083d342984164a83c1e4839aebce82041869a0a8
@graphql-codegen/typescript-resolvers@4.2.0-alpha-20240625114132-083d342984164a83c1e4839aebce82041869a0a8

Please let me know how these go!

mx-bernhard commented 3 months ago

Unfortunately I don't have time for the issue this week. I'll get back to it at the beginning of next week

mx-bernhard commented 3 months ago

Looks good! I don't find any other issues anymore.

🚀

eddeee888 commented 3 months ago

Thanks for reporting and working with me on this. This is fixed in @graphql-codegen/typescript-resolvers@4.2.1

mx-bernhard commented 3 months ago

Great work!

hbj commented 2 months ago

Is there a reason why we don't use the resolver types for all non scalar properties?

For example for the following schema:

type A {
  a: String
}

type B {
  b: String
}

type C {
  a: A
  b: B
}

we get the following resolver types (with defaultMapper: 'Partial<{T}>'):

export type ResolversTypes = ResolversObject<{
  A: ResolverTypeWrapper<Partial<A>>;
  B: ResolverTypeWrapper<Partial<B>>;
  C: ResolverTypeWrapper<Partial<C>>;
}>;

Why not have C: ResolverTypeWrapper<Partial<Omit<C, 'a' | 'b'> & { a: ResolversTypes['A'], b: ResolversTypes['B'] }>>; instead?

At the end when we're resolving a field of type C we will also be resolving a and b.

eddeee888 commented 1 month ago

Hi @hbj ,

Could you please open a new issue here and tag me: https://github.com/dotansimha/graphql-code-generator/issues/new?assignees=&labels=&projects=&template=bug_report.yml

It'll help me track and prioritise issues

dotansimha / graphql-code-generator