graphql icon indicating copy to clipboard operation
graphql copied to clipboard

Make `@ResolveField` generic, offering type safety

Open sinclairnick opened this issue 3 years ago • 4 comments

Is there an existing issue that is already proposing this?

  • [x] I have searched the existing issues

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

Consider the GraphQL entity Person:

@ObjectType()
class Person {
  @Field()
  id: string;
  @Field()
  name: string;
  @Field(() => [Friend])
  Friends: Friend[] // This is a relation we need to resolve
}

When writing the resolver for Person, our @ResolveField usage looks something like

  //...
  @ResolveField(() => [Friend], { name: "may-not-exist" })
  findFriends(@Parent() parent: Person){
    //...
  }
  //...

Here the name for the resolver is an arbitrary string, but in reality it must be specified on the entity for person. Hence the @ResolveField usage is coupled to the Person entities fields. However, we can write whatever we want in the name field, creating room for error.

Describe the solution you'd like

A relatively cost-free solution to this could be to use a generic @ResolveField function, like:

import {
  ResolveField,
  ResolveFieldOptions,
  ReturnTypeFunc,
} from "@nestjs/graphql";

export const ResolveFieldTyped = <T extends { [key: string]: any }>(
  typeFunc?: ReturnTypeFunc,
  options?: Omit<ResolveFieldOptions, "name"> & { name: keyof T & string },
) => {
  return ResolveField(typeFunc, options);
};

Here, the name field is, optionally, typed to the entity you're aiming to resolve. This means our field resolvers are statically checked to be valid before needing to run our schema, and perhaps run into silently incorrect code.

Teachability, documentation, adoption, migration strategy

It is backward compatible, so would not need any of this other than demonstrating the use of a generic

What is the motivation / use case for changing the behavior?

Additional type safety, enforcing safer code

sinclairnick avatar Feb 03 '22 23:02 sinclairnick

Would you like to create a PR for this issue?

kamilmysliwiec avatar Feb 04 '22 07:02 kamilmysliwiec

@kamilmysliwiec Yep, I'll submit one in the next couple days

sinclairnick avatar Feb 08 '22 02:02 sinclairnick

🏓 🙌

kamilmysliwiec avatar May 20 '22 09:05 kamilmysliwiec

I add a bit of my thoughts. Actually, you don't need to have your calculated fields (@ResolveField() in resolver) in your model. The framework will merge declarations from your entity + all @ResolveField() found in a project.

And I personally encourage developers to not put computed fields in the model definitions and below i will explain why.

It's actually bring more typesafety than proposed in this issue changes.

@ObjectType()
class Person {
  @Field()
  id: string;
  @Field()
  name: string;
  @Field(() => [Friend])
  Friends: Friend[]; // This is a relation we need to resolve
}

// person.resolver.ts
@Resolver(() => Person)
class PersonResolver {
  @Query(() => Person)
  async getOnePerson(
    id: string,
  ): Promise<Person> {
    const dbPerson = await personLoader.load(id);

    return {
      id: dbPerson.id,
      name: dbPerson.name,
      Friends: [], // <--- here i should return some fake value for field. Or mark return type as Partial<Person>
    };
  }

  @ResolveField(() => [Friend], { name: 'may-not-exist' })
  findFriends(@Parent() parent: Person) {
    // In child resolvers typescript will not help future developers to understand 
    // which fields really passed from parent and which are computed on the fly
    parent.Friends; // <-- no error, no help from compiler.
  }
}

It is much better when resolved fields not specified in the entity:

@ObjectType()
class Person {
  @Field()
  id: string;
  @Field()
  name: string;
}

// person.resolver.ts
@Resolver(() => Person)
class PersonResolver {
  @Query(() => Person)
  async getOnePerson(
    id: string,
  ): Promise<Person> {
    const dbPerson = await personLoader.load(id);

    // return object which exactly followed Typescript type, no Partial types involved
    return {
      id: dbPerson.id,
      name: dbPerson.name,
      // no need to return empty values or null (for possibly non nullable fields)
      // Friends: []
    };
  }

  @ResolveField(() => [Friend], { name: 'may-not-exist' })
  findFriends(@Parent() parent: Person) {
    parent.Friends; // <-- would be error. Typescript types in child resolvers actually reflects runtime objects 
  }
}

timofei-iatsenko avatar Sep 09 '22 08:09 timofei-iatsenko