type-graphql icon indicating copy to clipboard operation
type-graphql copied to clipboard

InputType vs ArgsType + Architectural Discussion

Open desmap opened this issue 5 years ago • 12 comments

I'm looking for a reference architecture in order to have all entities described consistently and came up with following. Something like this could be also helpful in the docs but yeah, first I wanted to discuss if this makes sense.

Example blog post entity:

  1. interface IPost => what's is needed to interact with a Post instance (this could be anything such as all CRUD operations)
  2. class PostInput implements IPost => what's is needed to mutate a Post instance
  3. class Post implements IPost => what's needed to persist a Post instance

The GraphQL surface is always <= than above (so it's equal or a subset).

1 und 2 are more flexible than 3, means e.g. 1. and 2. can consist of optional properties only: When I create a new blog post, there's no need for any input, the idcan come from the DB and initially the post content is empty. For 3, we need to have at least the id as primary key.

Now, I am wondering where @ArgsType() would fit in, I've two options:

(A) Like in the current docs: A dedicated class PostInput and dedicated classes [specific use case]PostArgs, e.g. class GetTodaysPostsArgs, one for every resolver:

@InputType()
class PostInput implements IPost

@ArgsType()
class GetTodaysPostsArgs implements IPost

...

(B) Or just one class PostInput both decorated @InputType() and @ArgsType(); since class PostInput is flexible, every use case should fit, it's DRY and should still give us some type safety, use always class validations (not so if I'd use @Arg from time to time), so something like...

@InputType()  
@ArgsType()  
class PostMutate implements IPost

Does this make sense?

desmap avatar Jun 05 '20 11:06 desmap

I would go with something different: https://github.com/MichalLytek/type-graphql/tree/master/examples/mixin-classes

  1. Create base mixin for simple payload-like properties (user name, email, etc.)
  2. Then build a dedicated entity/object type class from that mixin with id property and advanced field resolvers
  3. Then build from the mixin a decicated input type
  4. You can also construct easier from mixin a dedicated args type for filtering, etc.

MichalLytek avatar Jun 05 '20 12:06 MichalLytek

Ok, sounds cools, fwiw I charted your example to get a better feeling:

image

  1. You used a different mixin style than in the TS handbook https://www.typescriptlang.org/docs/handbook/mixins.html they use some fancy interface declaration merging what's better and why did you choose your style?
  2. So, actually I should break down everything into atomic classes (as atomic as required) and orchestrate them per use case which can be anything (interacting, mutation, persistence)?!
  3. Then, it shouldn't be too much effort to have dedicated Input- and ArgTypes since I just put them together like Legos and have hence granular type safety per resolver?!

Does this reflect your thoughts?

desmap avatar Jun 05 '20 12:06 desmap

You used a different mixin style than in the TS handbook typescriptlang.org/docs/handbook/mixins.html they use some fancy interface declaration merging what's better and why did you choose your style?

TypeScript mixins docs was created before ES2015 which introduced class mixins to JS. The TypeScript mixins doesn't work well, so it's recommended to use class mixins: https://basarat.gitbook.io/typescript/type-system/mixins

Does this reflect your thoughts?

Yes, you should compose the dedicated types from the legos. In the future (vNext) it will be more powerful with no @XYZType inheritance limitation and partial/required type operators.

MichalLytek avatar Jun 05 '20 13:06 MichalLytek

When is vNext going to happen approximately? 😃

Scott

smolinari avatar Jun 05 '20 13:06 smolinari

Between 2020 and 2025 😉

MichalLytek avatar Jun 05 '20 13:06 MichalLytek

Ok, gonna rebuild my stuff as below. Feedback or anything which makes the code maintainable/future-proof/fun is welcome:

image

desmap avatar Jun 05 '20 14:06 desmap

One thing is bothering me though: Let's take the mixin class Id: I want that id is optional on class NewPostInput but mandatory on class Post and class GetPost.

How would I model this DRY? Or do I need to two mixins such as class Id and class IdOptional which I mixin depending on the respective classes?

desmap avatar Jun 05 '20 16:06 desmap

How would I model this DRY? Or do I need to two mixins such as class Id and class IdOptional which I mixin depending on the respective classes?

In the future (vNext) it will be more powerful with (...) partial/required type operators.

MichalLytek avatar Jun 05 '20 16:06 MichalLytek

Between 2020 and 2025 😉

Doh! 🤦‍♂️ 😁

Scott

smolinari avatar Jun 05 '20 18:06 smolinari

So, I found another thing with the current architecture: I use constructors b/c I need to pass express' req object in them since my app is hostname-specific. When I now mixin classes with type-graphql decorators type-graphql/dist/schema/schema-generator.js gets executed without an actual request leading to an error because of the lacking request object.

Otherwise it feels quite slick but I have to solve somehow this constructor args passing...

desmap avatar Jun 06 '20 06:06 desmap

You should put request/caller related data into GraphQL context and then retrive it in resolvers via @Ctx() decorator.

Then you treat the value in you business logic just like a normal value from file or from database, pass it as a parameters to services, etc.

MichalLytek avatar Jun 08 '20 17:06 MichalLytek

Hey, sorry for my late reply and thanks for the hint! It required me to rebuild my classes but that was the solution: Before I used new Class() to instantiate new ActiveRecord-like objects which can save themselves etc. but this was too much magic and again required the Context object at instantiation.

So, I just implemented your idea and now every data manipulation happens exclusively over static methods in a class and new Class() is only used to create the instance for run-time validations which is then passed to the DB. IDK if this will give me any road-blockers in future but at the moment it's a satisfying architecture considering all the restrictions/complexity.

desmap avatar Jun 13 '20 08:06 desmap