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

Adds type generation for the resolvers in the GraphQL API #4786

Closed
wants to merge 4 commits into from

Conversation

orta
Copy link
Contributor

@orta orta commented Mar 17, 2022

One of the tensions I felt with my resolvers was a lack of cohesion between the generated templates and the type system represented in the schema. For example the default template recommends using something like:

import type { Prisma } from '@prisma/client'

export const user = ({ id }: Prisma.UserWhereUniqueInput) => {
  return db.user.findUnique({
    where: { id },
  })
}

interface CreateUserArgs {
  input: Prisma.UserCreateInput
}

export const createUser = ({ input }: CreateUserArgs) => {
  return db.user.create({
    data: input,
  })
}

Where the input comes from Prisma's type system, and not graphqls - this is kinda right, most of the time but also not right enough that I started to lose time to warrant figuring out a better answer. This PR is my answer, I mentioned it in #4802 (I still think root should be called obj FWIW)

My solution is to make a new file in the types directory types/resolvers whose job is to take the SDL and generate types which match the Redwood resolver shape. The above code would now be:

import type { Queries, Mutations } from 'types/resolvers'

export const user: Queries['user'] = ({ id }) => {
  return db.user.findUnique({
    where: { id },
  })
}

export const createUser: Mutations['createUser'] = ({ input }) => {
  return db.user.create({
    data: input,
  })
}

Which is less code, and I think a bit clearer in the intent.

The JS output can also be improved...

I didn't do it, but for JS users you could use JSDocs to make the tooling work:

/** @type {import("types/resolvers").Queries["user"] */
export const user = ({ id }) => {
  return db.user.findUnique({
    where: { id },
  })
}

/** @type {import("types/resolvers"). Mutations["createUser"] */
export const createUser = ({ input }) => {
  return db.user.create({
    data: input,
  })
}

I'll leave that for someone else to figure out.

@netlify
Copy link

netlify bot commented Mar 17, 2022

✔️ Deploy Preview for redwoodjs-docs canceled.

🔨 Explore the source changes: e4d9e94

🔍 Inspect the deploy log: https://app.netlify.com/sites/redwoodjs-docs/deploys/623338340c196f0009359b3d

@dthyresson dthyresson added the release:feature This PR introduces a new feature label Mar 17, 2022
@orta orta marked this pull request as ready for review March 17, 2022 13:35
@thedavidprice
Copy link
Contributor

Thanks @orta!

@dac09 is this one you can take a look at?

@dac09
Copy link
Collaborator

dac09 commented Mar 18, 2022

Hey @orta thank you so much for this.

We spoke recently to the Guild, and it seems the "mapped types" for Prisma is something they support in graphql-codegen - see this blog here: https://www.the-guild.dev/blog/graphql-code-generator-and-prisma

I haven't had time to explore this, but what do you think of configuring it as per the blog in addition to customizing the resolver shape (like this: #4802)? How would that compare to the solution in this PR (apart from the separate file being generated)?

@orta
Copy link
Contributor Author

orta commented Mar 18, 2022

You'd still have to write something like:

export type GQLResolverToRedwoodResolver<T, CTX = {}> = {
  [Property in keyof T]: (
    resolverArgs: Parameters<T[Property]>[1],
    processing: {
      root: Parameters<T[Property]>[0]
      context: {
        document: DocumentNode
        operation: OperationDefinitionNode
        variables: any
      } & RedwoodGraphQLContext &
        CTX
      info?: GraphQLResolveInfo | string
    }
  ) => ReturnType<T[Property]>
}

Because Redwood's resolver format isn't the graphqljs format, but I'm not really sure what that post is trying to say to be honest. So maybe you should give that a quick prototype and compare the results

@orta
Copy link
Contributor Author

orta commented Mar 25, 2022

This needs some work to handle strict mode in TypeScript because all the types in the generated API are optionals:

export type QueryResolvers<ContextType = any, ParentType extends ResolversParentTypes['Query'] = ResolversParentTypes['Query']> = {
  accounts?: Resolver<Array<ResolversTypes['Account']>, ParentType, ContextType>;
  activities?: Resolver<ResolversTypes['ActivityConnection'], ParentType, ContextType, Partial<QueryactivitiesArgs>>;
  activity?: Resolver<Maybe<ResolversTypes['Activity']>, ParentType, ContextType, RequireFields<QueryactivityArgs, 'id'>>;

I also took a look at that article, they recommend removing nulls from GraphQL inputs which is a bad idea - I've lost a bunch of data a lot of times because null wasn't accounted for in the type system differences between graphql and prisma

WRT the model importer, hard to say if that's a good idea without seeing it in action - doesn't feel right to me from their description though


The two hardcoded examples need to be Required<X> to strip the ?

@dac09
Copy link
Collaborator

dac09 commented Mar 25, 2022

Hey sorry man, I've not had a chance to look at this at all in the rush for v1. This one is likely a patch/minor release after - and I will come back to it!

In terms of customizing the resolver shape, we can change the resolver type with config similar to this: https://www.graphql-code-generator.com/plugins/typescript-resolvers#with-graphile

So in our case it would look like:

    config:
      customResolverFn: |
      (
            args: TArgs,
            {
              root,
              context,
              info
            }: { root: TParent; context: TContext; info: GraphQLResolveInfo }
          ) => Promise<Partial<TResult>> | Partial<TResult>;

You can try this out by adding codegen.yml to your project, pasting the above and regenerating the types.

With regards to mapping - I will take your advice here on what the right thing to do is. We can still use your mapper, configured in codegen maybe?

plugins
  plugins:
    - 'typescript'
    - 'typescript-resolvers'
    - add:
        content: "import { OrtaMapper } from '@redwoodjs/api" #or somewhere more approriate
  config:
    defaultMapper: OrtaMapper<{T}>

If you have some spare time, it would probably help to connect real time to go through our options!

@dac09
Copy link
Collaborator

dac09 commented Apr 28, 2022

Closing this in favour of #5216 - thank you so much for this @orta 🎸 - and also for providing the alternative approach.

If we think later on that my preference for the other PR was wrong, I'll make sure to dig out this PR and switch to this approach!

@dac09 dac09 closed this Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants