stepci / garph

Fullstack GraphQL Framework for TypeScript
https://garph.dev
MIT License
1.31k stars 17 forks source link

How to handle field level resolvers? #48

Closed christopher-caldwell closed 1 year ago

christopher-caldwell commented 1 year ago

Say you have a relationship Authors to Books. The Book type has a field author, which could be a field level resolver to avoid the n+1 problem. Defining the schema for this would be like:

const authorType = g.type('Author', {
  name: g.string(),
})

const bookType = g.type('Book', {
  title: g.string(),
  author: g.ref(bookType)
})

const queryType = g.type('Query', {
  books: g.ref(authorType).list(),
})

The resolver would be something like:

const resolvers: InferResolvers<{ Query: typeof queryType }, {}> = {
  Query: {
    async books(parent, args, context, info) {
      const allBooks = db.findAllBooks()
      // does not include author - TS error saying it's missing `author`
      return allBooks
    },
  },
}

Is it possible for this to be setup to know that there's a field level resolver called author somewhere that will satisfy the requirement? As is, this is a TS error, and I have to use a DeepOmit utility type to ignore the need for books.

So far, I have thought about it being optional, but in reality, it's not optional. The schema needs to say this is required, but the resolver needs to be allowed to return without it. This is super challenging for TS to know there's a field resolver somewhere, but at least there could be something like g.fieldResolver() which makes the field optional when using InferResolvers. If someone leaves it off, it will throw a GraphQL error.

christopher-caldwell commented 1 year ago

If it's helpful, I can make a sample repo showing the issue.

mishushakov commented 1 year ago

You're welcome to contribute an example! I'm also considering adding resolvers directly to the fields

mishushakov commented 1 year ago

Also, could you try to do the same thing using SDL and graphql-codegen and tell me whether the result matches what you expected? eg. Did you still have to use DeepOmit there?

christopher-caldwell commented 1 year ago

Okay, I will put together an example.

I have the same issue with codegen, and it's the origin of the DeepOmit lol. So it's not something that's lacking, more so trying to figure out the best way to make it work.

mishushakov commented 1 year ago

If the behaviour is the same as with codegen, then it's "correctly" implemented on our side too. This doesn't mean we can't make improvements though πŸ˜„

Will check out your example once it's ready and see what we can do Thanks

christopher-caldwell commented 1 year ago

@mishushakov https://github.com/christopher-caldwell/garph-field-resolver-demo

If I left anything out, happy to clear it up! Thanks for taking the time to look.

mishushakov commented 1 year ago

Great, will take a look!

christopher-caldwell commented 1 year ago

Thanks!

christopher-caldwell commented 1 year ago

Any ideas?

mishushakov commented 1 year ago

Will see on the weekend. Thanks for the reminder in any case!

mishushakov commented 1 year ago

Sorry, I was a bit overwhelmed last weekend. I will you know when I have something πŸ™

christopher-caldwell commented 1 year ago

Thank you!

christopher-caldwell commented 1 year ago

I can try to contribute if you point me in the right direction.

erickreutz commented 1 year ago

What I believe @christopher-caldwell is talking about is the issue and question I am running into as well.

As an example illustrated below - the profile doesn't exist on the object but is a derived field from the resolver so returning { id: string } doesn't satisfy the expected return type { id: string, profile: Profile }. What should we return here? I'd hope to not have to cast it.

const User = g.type("User", {
  id: g.id(),
  profile: g.ref(Profile)
});

const Profile = g.type("Profile", {
  name: g.string(),
  bio: g.string(),
});

const Query = g.type("Query", {
  viewer: g.ref(User).optional()
});

const resolvers: InferResolvers<{ Query: typeof Query }, { context: Context, User: typeof User }> = {
  Query: {
    viewer: (root, args, context, info) => {
      /*
       TypeScript Error
       Property 'profile' is missing in type 'User' but required in type '{ __typename?: "User" | undefined; id: string; profile: { __typename?: "Profile" | undefined; name: string; bio: string; }; }'
      */
      return context.currentUser ?? null;
    }
  },
  User: {
    profile: async (parent, args, context, info) => {
      return await db.findProfileByUserId(parent.id);
    }
  }
};
christopher-caldwell commented 1 year ago

@erickreutz I think you are missing the Infer generic.

Taking your snippet, this works for me:

import { g, Infer, InferResolvers } from 'garph'

interface Context {
  currentUser: Infer<typeof User>
}
const Profile = g.type('Profile', {
  name: g.string(),
  bio: g.string(),
})

const User = g.type('User', {
  id: g.id(),
  profile: g.ref(Profile),
})

const Query = g.type('Query', {
  viewer: g.ref(User).optional(),
})

export const resolvers: InferResolvers<{ Query: typeof Query }, { context: Context }> = {
  Query: {
    viewer: (root, args, context, info) => {
      return context.currentUser ?? null
    },
  },
}

I also don't think these issues are related. Again taking your example, my issue is that I'd like to define the User in one file, and the Profile in another. This isn't currently possible, unless you pass a singular reference to g through function arguments in a factory pattern.

Edit: My bad, I was referencing a different issue.

erickreutz commented 1 year ago

@christopher-caldwell Hmm I'm not sure you are following then. currentUser is not a graphql object. It's a record from the db representing the current user of the request. Illustrating an example that doesn't use the context results in the same issue.

const User = g.type('User', {
  id: g.id(),
  firstName: g.string(),
  lastName: g.string(),
  fullName: g.string(), // this is a resolved field
})

const Query = g.type('Query', {
  viewer: g.ref(User).optional(),
})

export const resolvers: InferResolvers<{ Query: typeof Query, User: typeof User }, {}> = {
  Query: {
    viewer: (root, args, context, info) => {
      // Property 'fullName' is missing in type '{ id: string; firstName: string; lastName: string; }' but required in type '{ __typename?: "User" | undefined; id: string; firstName: string; lastName: string; fullName: string; }
      return {
        id: '1',
        firstName: 'John',
        lastName: 'Doe',
      }
    },
  },
  User: {
    fullName: (root, args, context, info) => {
      return `${root.firstName} ${root.lastName}`;
    }
  }
}

Much like how your author is not part of the Book and is resolved fullName is not part of User but is resolved. As in my first example profile is not part of User but is resolved. We need some way to mark a field as being resolved something like g.resolved(g.string()) or g.string().resolved() or define the resolvers inline with the type.

christopher-caldwell commented 1 year ago

@erickreutz Ahh okay! Yeah, I understand now. My bad.

Yeah having a way to mark the field as resolved by something else, exactly what you illustrate here, is my issue as well.

In a perfect world, if you marked a field as such (resolved or field level, whatever makes sense) and you did not declare it in the User resolvers block, it should throw a TS error.

const User = g.type('User', {
  id: g.id(),
  firstName: g.string(),
  lastName: g.string(),
  fullName: g.string().fieldLevel() // whatever name makes sense
})

const resolvers: InferResolvers<{ Query: typeof Query, User: typeof User }, {}> = {
  Query: {
    viewer: '...'
  },
  User: {
   // If this was omitted, but declared above as a field level resolver, TS should throw.
    fullName: (root, args, context, info) => {
      return `${root.firstName} ${root.lastName}`;
    }
  }
}
christopher-caldwell commented 1 year ago

Anything new here?

mishushakov commented 1 year ago

Hey, just looked into this more deeply

Here are my findings:

Sorry for the wait 😁

mishushakov commented 1 year ago

Here's a demo of upcoming omitResolver. Once specified you can now skip resolving this value in the root resolver. You can combine it with InferStrict

https://github.com/stepci/garph/assets/10400064/4470f25e-88f1-4dfc-a930-3be2f644f6fc

Let me know if you have any feedback?

mishushakov commented 1 year ago

.omitResolver was added in 0.5.4