redwoodjs / redwood

The App Framework for Startups
https://redwoodjs.com
MIT License
17.25k stars 991 forks source link

Re-consider calling the graphql resolvers first arg as 'root' from 'obj' #4802

Open orta opened 2 years ago

orta commented 2 years ago

I'm sure you had this debate at some point, but root is only occasionally accurate, and often wrong:

export const User = {
  accounts: (_obj, { root }: ResolverArgs<ReturnType<typeof user>>) => db.user.findUnique({ where: { id: root.id } }).accounts(),
}

root is somewhat right for the query, which (if you ignore the root Query object), the 'root' of the query could be argued to be accurate:

getUser(id: $id) {
     accounts {
        id
     }
 }

but realistically the moment it's a subquery

getGroup(id: $id) {
     users {
           accounts {
              id
           }
     }
 }

It's inaccurate anyway, but then you don't necessarily want to be thinking too much about the position of your query during the creation of a resolver anyway. Given that the param is meant to represent the current object that the resolver's function is meant to run within obj is generally the right term even if it is vague.


Perhaps a more ideal solution would be to maybe rename it to the name of class

export const User = {
  accounts: (_obj, { user }: ResolverArgs<ReturnType<typeof user>>) => db.account.findUnique({ where: { id: user.id } }).accounts(),
}

This is possible with work, but it'd require a bit of magic and some codegen to copy the types represent that)


IMO it should be renamed back to obj (and keep root in the object for compat till Redwood 2.0)

dthyresson commented 2 years ago

FYI, because Redwood generates types and relies on

export declare type ResolverArgs<TRoot> = {
    root: ThenArg<TRoot>;
};

@orta would you be ok with

export const User = {
  accounts: (_obj, { root: user }: ResolverArgs<ReturnType<typeof user>>) => db.user.findUnique({ where: { id: user.id } }).accounts(),
}

Otherwise, I may have to generate types per model/resolver.

Also, do you think a short comment above this section will help explain?

TODO: See if _obj can have a more informative name and/or link

Tobbe commented 2 years ago

Otherwise, I may have to generate types per model/resolver.

I don't think this is unreasonable. It's what I would have done had I written all this manually.

dac09 commented 2 years ago

@dthyresson ~I think we can change the type here - it might be better to use the type from the generated graphql types - this is already done in api/graphql.d.ts~

Removing comment, was wrong!

dthyresson commented 2 years ago

I could do

// ...
export const deletePost = ({ id }: Prisma.PostWhereUniqueInput) => {
  return db.post.delete({
    where: { id },
  })
}

type ThenArg<T> = T extends PromiseLike<infer U> ? U : T

export type PostResolverArgs<TRoot> = { post: ThenArg<TRoot> }

export const Post = {
  author: (_obj, { post }: PostResolverArgs<ReturnType<typeof post>>) =>
    db.post.findUnique({ where: { id: post.id } }).author(),
}

Is that nicer? Or more ts clutter?

corbt commented 2 years ago

Really excited you're looking at this @dthyresson!

I think the ideal typing experience for me would be a single generated type for each resolver that includes types for all the arguments as well as the return.

Whatever is generating the file at api/types/graphql.d.ts kind of actually does that, but unfortunately it doesn't quite work for Redwood resolvers as-is since it expects the resolver function arguments to follow the GraphQL resolver spec, which Redwood departs from. But if we adjusted those generated types to match the order of Redwood type arguments, the following would work and provide an ideal typing experience:

import { MutationResolvers, PostResolvers } from 'types/graphql'

// The `id` parameter as well as the return type can be correctly typed by the generated `MutationResolvers['deletePost']` type
export const deletePost: MutationResolvers['deletePost'] = ({ id }) => {
  return db.post.delete({
    where: { id },
  })
}

// The arguments and return type of the `author` field can all be correctly typed based on the generated `PostResolvers` type
export const Post: PostResolvers = {
  author: (_obj, { post }) =>
    db.post.findUnique({ where: { id: post.id } }).author(),
}

If generating our own types ends up being too much work, there may be a less-pretty but probably much easier-to-implement solution that just involves writing a type adapter for the existing generated types. It would look something like this:

import type { MutationResolvers, QueryResolvers, PostResolvers } from 'types/graphql'

// These are the new type adapters that would need to be written
import type { RWMutation, RWQuery, RWObjectResolver }

export const deletePost: RWMutation<MutationResolvers['deletePost']> = ...

export const posts: RWQuery<QueryResolvers['posts']> = ...

export const Post: RWObjectResolver<PostResolvers> = ...

Hope that makes sense!

jtoar commented 2 years ago

@dthyresson not sure if you have another issue tracking this already (let me know if so); if not, I just added it to the release board for a little more visibility.

dthyresson commented 2 years ago

@dac09 Let's pair on this for your TS wizardry, perhaps?

dac09 commented 2 years ago

Hey @corbt thanks for your suggestion, and to me it seemed very elegant.

So I managed to configure the type generator to change the shape of the resolver function to match Redwood's, but there's one issue which means its a no-go.

Let's say we have sdls like this:

gql`
type User {
    id: Int!
    email: String!
    name: String
    post: [Post]! # πŸ‘ˆ notice user type also has a post here
  }

  type Post {
    id: Int!
    title: String!
    body: String!
    user: User
    createdAt: DateTime!
    userId: Int
  }
`

So the User type ends up being generated as:

export type User = {
  __typename?: 'User';
  email: Scalars['String'];
  id: Scalars['Int'];
  name?: Maybe<Scalars['String']>;
  post: Array<Maybe<Post>>; // πŸ‘ˆ problem
};

If we try to define export const Post: PostResolvers = { it complains because the Post.user doesn't contain post. I'm a little at a loss on how to work around this problem.

corbt commented 2 years ago

Hmm yeah, that does seem hard. There might be some way to do it if the type of PostResolver references UserResolver instead of User?

Another alternative would be to have all fields be optional in the generated types, which is a capitulation and makes the types less useful (since they won't warn you if you forget to implement a field) but still a lot better than nothing.

dac09 commented 2 years ago

Another alternative would be to have all fields be optional in the generated types, which is a capitulation and makes the types less useful (since they won't warn you if you forget to implement a field) but still a lot better than nothing.

That's what I ended up trying. I suppose in Partial is the simplest solution - but like you noted definitely a workaround . If you're curious this is the shape of the custom resolver function:

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

I configured graphql-codegenerator to use this (https://www.graphql-code-generator.com/plugins/typescript-resolvers#usage-examples-3).

Just to add to our misery, I've found yet another issue - if your model contains a DateTime (like createdAt), prisma will return Date, while the generate graphql types are expecting a string (we map Scalar['DateTime'] to string), and you end up getting "Type 'Date' is not assignable to type 'string'.ts(2322)"

orta commented 2 years ago

I ended up with incorrect input types in some of my resolvers and thought I'd come back to this as it seems to be the most on-topic discussion.

I did some type-foo to automatically generate the redwood style resolvers from the ones in graphql.d.ts, this lives in api/types/resolvers.d.ts for me:

import type { MutationResolvers, QueryResolvers } from "types/graphql"
import type { RedwoodGraphQLContext } from "@redwoodjs/graphql-server/dist/functions/types"
import type { OperationDefinitionNode } from "graphql"

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]>
}

export type RedMutations = GQLResolverToRedwoodResolver<MutationResolvers>
export type RedQueries = GQLResolverToRedwoodResolver<QueryResolvers>

Then I changed my root resolvers to:

export const updateSomethingState: RedMutations["updateSomethingState"] = async (args, processing) => {

or

export const adminUsersConnection: RedQueries["adminUsersConnection"] = async (args, processing) => {

Which correctly approximates the input/output.

orta commented 2 years ago

WRT to the tension from the types expecting GraphQL types and we're returning prisma models, an option could be for Redwood to make a types map during codegen and switching out ReturnType<T[Property]> with something among that map. As a rough example

import type {User} from "types/graphql"
import type {User: PrismaUser} from "@prisma/client"

type GQLTaPrismaMap = {
  User: PrismaUser
}

Then with something like the above: ) => ReturnType<T[Property]> would want to use that map to have the resolvers always declare that they return the underlaying prisma version of the graphql model.

orta commented 2 years ago

I didn't really think about it till I started getting to the end of porting the entire app to use those types (because Query and Mutation are just objects in a GQL server), but they also apply to the per-object resolvers commonly found at the bottom of each service files too:

export type ObjQueries = GQLResolverToRedwoodResolver<ObjectiveInstanceResolvers, { gamePlayUserID: string }>

export const ObjectiveInstance: ObjQueries = {
  definition: (_args, { root }) => db.objectiveInstance.findUnique({ where: { id: root.id } }).definition(),

  userProgress: (_args, info) => {
    const id = context.currentUser?.id || info.context.gamePlayUserID
    return db.objectiveProgress.findFirst({ where: { ownerID: id, instanceID: info.root.id } })
  },

  allProgressForInstance: (args, { root }) => {
    findManyCursorConnection(
      (args) => db.gamePlayed.findMany({ ...args, where: { ownerID: root.id } }),
      () => db.gamePlayed.count({ where: { ownerID: root.id } }),
      args
    )
  },
}
cjreimer commented 2 years ago

I just wanted to poke my head in here and say I appreciate this discussion! Our graphQL side of some services significantly different than the Prisma side, so creating types from the graphQL interface makes sense for us. Right now we are using the generated types from the graphQL side, but with manual manipulation to make them work. It would be great if some of the "magic" could be automated and standardized.

@dac09 indicated above a problem with the Dates being Scalars['DateTime'] which resolve to string on the graphQL side and Prisma returns a Date. Right now we are moving to utilize the following to help resolve that issue; however, I'm sure some of the typescript wizards here have figured out something better already.

export type ResolverDateType<T extends object> = {
  [key in keyof T]: T[key] extends Scalars['DateTime'] ? Date | string : T[key]
}

Anyway, looking forward to seeing where this all goes!