Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query object resolvers type should require only those fields not already required on the mapped type from resolvers #9887

Closed
loucadufault opened this issue Feb 28, 2024 · 4 comments
Assignees

Comments

@loucadufault
Copy link

loucadufault commented Feb 28, 2024

Is your feature request related to a problem? Please describe.

Mappers are useful for situations where the a resolver for a Query field returns a partial object, where some fields are populated by resolvers defined directly on that fields type in the schema.

With this simple schema:

  type Book {
    title: String!
    author: Author!
  }

  type Author {
    name: String!
    country: String!
  }

  type Query {
    books: [Book]
  }
// src/index.ts
import { QueryResolvers, Resolvers } from './generated/resolverTypes.ts'

const booksResolver: QueryResolvers['books'] = () => [
  {
    title: 'The Awakening',
    authorName: 'Kate Chopin',
  },
  {
    title: 'City of Glass',
    authorName: 'Paul Auster',
  },
]

const resolvers: Resolvers = {
  Query: {
    books: booksResolver,
  },
  Book: {
    author: (parent) => {
      return authors.find((author) => author.name === parent.authorName)
    }
  }
};

type BookModel = {
  title: string
  authorName: string
}

We would define mappers for codegen as:

// codegen.ts
export default {
  ...
  generates: {
    'src/generated/resolversTypes.ts': {
      config: {
        mappers: {
          Book: 'src/index.ts#BookModel',
        },
        ...

The issue is that the resolver for the Book object in the resolvers is typed as:

  1. having all fields optional, if avoidOptionals is false
  2. having all fields required, if avoidOptionals is true

In this case with the mapped type, neither option is accurate to what is necessary to have a complete implementation of the schema. With it not set, there is no check that the author field has any resolver. With it set, there are excess checks for resolvers that are not needed.

The more type safe option is to set the option to true, which will require the resolvers for the Book object to include a resolver for each field of the Book type.

Describe the solution you'd like

However, codegen knows that every resolver defined in the schema to return an object of the given type, will return an object of the mapped type. Therefore it could generate an appropriate type on the Resolvers for that Object type that would require only resolvers for the fields missing from the mapped type. This would be taken as the (recursive) difference between the schema type and the mapped type.

In this example, codegen knows that every resolver returning a Book Object will return an object of type BookModel, therefore missing only the field author, which should be specified on the resolvers for the Book object type.

Alternatively, the config should allow specifying a separate mapped type for the Object type on the Resolvers, to at least allow doing this reconcilation manually.

Describe alternatives you've considered

Current workaround is to manually type it:

const resolvers: Resolvers = {
  Query: {
    books: booksResolver,
  },
  Book: {
    author: (parent) => {
      return authors.find((author) => author.name === parent.authorName)
    }
  } satisfies Pick<NonNullable<Resolvers['Book']>, keyof Omit<Book, keyof BookModel>>
};

Is your feature request related to a problem? Please describe.

No response

@loucadufault loucadufault changed the title Query object resolvers type should require only those fields not already required on the mapped type for that as resolvers for types at the top level Query object resolvers type should require only those fields not already required on the mapped type from resolvers Feb 28, 2024
@eddeee888
Copy link
Collaborator

eddeee888 commented Apr 27, 2024

Hi @loucadufault ,
On top of the "fields missing from the mapped type must be required" scenario, there are a few cases we need to consider:

  1. If the type of a mapper's field does not match the schema type's field with the same name, the field resolver must be impl. In the example below, isAdmin resolver is required in the following case because yes | no must be converted to boolean to avoid runtime error.
// schema
type User {
  isAdmin: Boolean!
}

// mapper
type UserMapper = {
  isAdmin: "yes" | "no"
}
  1. Mapper can be anything e.g. string, types from node_modules, inferred type, even a class

To be able to handle more complex cases effectively, we need to be able to do static analysis to compare the mapper against the generated TypeScript type. This can be done most effectively using the TypeScript compiler.

However, adding static analysis into the typescript-resolvers plugin may not be as effective as it sounds. One reason is performing static analysis in TypeScript could be complex, and adding it to the base plugin could make it much harder to maintain.

This is why Server Preset (@eddeee888/gcg-typescript-resolver-files) was created. It uses the generated types from typescript-resolvers plugins and compare it against the mapper types, handling the mentioned missing fields and other complex cases.

In your example, it'd automatically generate resolver files and mark the resolvers that need mappings, and tell the developer why it needs to be implemented:

export const Book: BookResolvers = {
  author: () => {
    /* Book.author resolver is required because Book.author exists but BookMapper.author does not */
  },
 }

Here's another demo where server preset adds required resolvers on mapper type changes:

server-preset.mov

There's more to features as well, you can read more on how it works and how to set it up here: https://the-guild.dev/graphql/codegen/docs/guides/graphql-server-apollo-yoga-with-server-preset

@eddeee888 eddeee888 self-assigned this Apr 27, 2024
@eddeee888
Copy link
Collaborator

Closing this because this is better addressed in Server Preset. If you have issues related to Server Preset, please create an issue in https://github.com/eddeee888/graphql-code-generator-plugins (It's in a separate repo but I'm working closely with the Codegen team)

@loucadufault
Copy link
Author

loucadufault commented May 19, 2024

Thanks for the reply

If the type of a mapper's field does not match the schema type's field with the same name, the field resolver must be impl.

Are these actual graphql-js semantics, that it will attempt to call the next resolver if the current one returned an unexpected type?

  1. Mapper can be anything e.g. string, types from node_modules, inferred type, even a class

To be able to handle more complex cases effectively, we need to be able to do static analysis to compare the mapper against the generated TypeScript type. This can be done most effectively using the TypeScript compiler.
However, adding static analysis into the typescript-resolvers plugin may not be as effective as it sounds. One reason is performing static analysis in TypeScript could be complex, and adding it to the base plugin could make it much harder to maintain.

Can you elaborate on this supposed complexity? To me it just seems like a deep type equality check, I can't imagine that is an unsolved problem in the TypeScript world. As far as checking for all the possible types, since TS types just wrap JS primitives, in this case an object, it comes down to checking properties.

I'm mainly not understanding the argument as to why this must be in a separate project; in my view this feature of codegen is fundamentally broken as it stands.

@eddeee888
Copy link
Collaborator

that it will attempt to call the next resolver if the current one returned an unexpected type?
The next resolver in the chain will always be called if provided. If the last one in the chain return a type that does not match the final output type, it'd send an error.

Can you elaborate on this supposed complexity?

Sure, here's one of the issues of doing it in base resolver plugin:

Generally, to effectively run TypeScript checks we need to load files into compiler's Program with compiler options. Then, we load files (virtual or physical), and do property checks, etc. Using the TypeScript compiler helps us do inferred type, type alias, interfaces, etc. mappers correctly

So, here's what we'd need to do:

  • The base resolver generates some output, but before we generate this output to physical files...
  • We need to load that output into the Program, load all mappers into the Program...
  • Do statical analysis to find missing fields...
  • Re-run codegen logic with missing fields to make them required...
  • Return the output to Codegen engine to generate the physical files

Apart from running the same logic twice, we need to use typescript or ts-morph as a hard dependency for the base resolver.

This will have performance impact to all existing users.

in my view this feature of codegen is fundamentally broken as it stands.

If you have ideas how to fix issues at the base plugin level, feel free to create a PR 🙂

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

No branches or pull requests

2 participants