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

Mark a @Field property as resolved by a @FieldResolver #440

Closed
dan-j opened this issue Oct 11, 2019 · 11 comments
Closed

Mark a @Field property as resolved by a @FieldResolver #440

dan-j opened this issue Oct 11, 2019 · 11 comments
Labels
Question ❔ Not future request, proposal or bug issue Solved ✔️ The issue has been solved

Comments

@dan-j
Copy link

dan-j commented Oct 11, 2019

Is your feature request related to a problem? Please describe.
We have @ObjectType() classes which are reused by the front end, however many properties are resolved by @Resolver() classes using @ResolveProperty() (we're using NestJS).

This means we have to ensure that our property resolver signature matches what's declared in the object type, and if they are different, which one gets used in the schema generation?

Describe the solution you'd like
An option on the @Field() decorator like, { expectFieldResolver: true } which tells type-graphql that this field should have a corresponding @FieldResolver() (or @ResolverProperty() in NestJS applications) with a matching signature.

If the property resolver doesn't exist on any @Resolver() class, or the signature is different, a warning/error should be emitted from the schema generation stage.

Describe alternatives you've considered
I've tried using the ResolverInterface interface but our @Root() type is an instance of our TypeORM @Entity() class and not an instance of our @ObjectType() class. We have tried ... implements ResolverInterface<MyEntity> so that @Root() is the correct type, but sometimes the field resolver returns an @ObjectType() instance (or a scalar), and other times it returns a related @Entity() class, which goes against the ResolverInterface definition.

Another solution I've considered is to just omit the @Field() definition on the @ObjectType() class but then these types are missing properties when they're imported by the UI.

@MichalLytek
Copy link
Owner

and if they are different, which one gets used in the schema generation?

The @FieldResolver overwrites the field type if type annotation provided.

I've tried using the ResolverInterface interface but our @root() type is an instance of our TypeORM @entity() class and not an instance of our @ObjectType() class.

So create your own interface that takes two parameters - object type and model type.

@MichalLytek MichalLytek added the Question ❔ Not future request, proposal or bug issue label Oct 11, 2019
@dan-j
Copy link
Author

dan-j commented Oct 11, 2019

The @FieldResolver overwrites the field type if type annotation provided.

This just did the opposite for me. I viewed the newly generated schema.graphql to check it had done what I was expecting and it had generated the type from the @ObjectType()/@Field() declaration, not the @FieldResolver(). I am using @ResolveProperty() from NestJS though, could this be a thing?

@dan-j
Copy link
Author

dan-j commented Oct 11, 2019

So create your own interface that takes two parameters - object type and model type.

Isn't this just moving the responsibility but not solving the problem? You still have to maintain a new type?

@MichalLytek
Copy link
Owner

MichalLytek commented Oct 11, 2019

What problem solving? Moving from compile-time to runtime with { expectFieldResolver: true }?

You still have to maintain a new type?

I can't maintain every use case of TypeGraphQL - maybe should I also add a third parameter with a list of allowed keys that you can oveerride the type?

With #193 you would be able to point the type and field that you want to implement resolver for.

@dan-j
Copy link
Author

dan-j commented Oct 11, 2019

I suppose I could have worded the OP better.

The suggestion of a new option expectFieldResolver was to mitigate the ambiguous results of schema generation because in my case, the schema was generated from the @Field() definition, not the field resolver which you mentioned should be the case. I'm also 99% certain I have experienced the opposite too, but can't find evidence right now.

I am using Nest's @ResolveProperty() and not @FieldResolver() though, could this be something to cause different behaviour?

@MichalLytek
Copy link
Owner

the schema was generated from the @field() definition

Yes, sorry for confusing - checked in the code and it uses types info only for creating new fields, not for implementing resolvers.

@dan-j
Copy link
Author

dan-j commented Oct 11, 2019

I can't maintain every use case of TypeGraphQL - maybe should I also add a third parameter with a list of allowed keys that you can oveerride the type?

Apologies in either case, but I don't know if this is serious or sarcasm.

I don't think changing the ResolverInterface is the answer. I do agree that to make it type safe, it should be the developer's responsibility to create and maintain that mapping interface/type and not the responsibility of this library.

Yes, sorry for confusing - checked in the code and it uses types info only for creating new fields, not for implementing resolvers.

In that case, you could do away with my suggestion of a new option and just warn if the type from @FieldResolver(() => NotSomeType) is different to @Field(() => SomeType)?

@dan-j
Copy link
Author

dan-j commented Oct 11, 2019

I've just noticed, we're actually discussing two separate problems which can't be solved with one solution.

  1. Type safety for the return type between @ObjectType() properties and @FieldResolver() methods, has to be done through the ResolverInterface interface, or some other custom interface.
  2. Safety that one @Field(() => SomeType) isn't conflicted by another @FieldResolver(() => NotSomeType) can't be mitigated by TypeScript. It's a type being used as a value within a decorator. For example you could quite easily solve 1) and then do this:
class ... {
  @FieldResolver(() => NotSomeType)
  someType(@Root() root): SomeType { ... }
}

Whatever solution you do for 1) to make the above type safe, you've still made a mistake that your @Field() and @FieldResolver() don't match and TypeScript will never know.

@MichalLytek
Copy link
Owner

MichalLytek commented Oct 11, 2019

I won't implement new runtime check option if the new syntax of pointing type and field solves that problem too and also allow for custom names of method:

@ObjectType()
class Author {
  @Field(type => [Post])
  posts: Post[]
}

@Resolver()
class Post Resolver
  @FieldResolver(of => Author, author => author.posts) 
  getAuthorPosts(@Root() author: Author, @Args() args: PostsArgs) {
    // ...
  }
}

@dan-j
Copy link
Author

dan-j commented Oct 11, 2019

Ahh, I read the issue you linked but at first glance didn't quite understand how it would solve the problem.

But yes, you're correct. You don't duplicate the field type with that new syntax so can never get it wrong 👍

@MichalLytek
Copy link
Owner

Great! So closing this issue as it will be fixed in #193 🔒

@MichalLytek MichalLytek added the Solved ✔️ The issue has been solved label Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question ❔ Not future request, proposal or bug issue Solved ✔️ The issue has been solved
Projects
None yet
Development

No branches or pull requests

2 participants