dangcuuson / graphql-schema-typescript

Generate TypeScript from GraphQL's schema type definitions
191 stars 36 forks source link

TResult type idea #8

Open jardakotesovec opened 6 years ago

jardakotesovec commented 6 years ago

Hi, I am trying out the push the types in resolvers to the limit with new options :-).

In my use case TParent is usually needed to retype to the model types (as often some additional fields that are not exposed via graphql are being used), but that still looks very good as it can be nicely defined per graphql type, like this :

  type Study {
    _id: ObjectId!
    ready: Bool!
    studyDescription: StudyDescription!
    referringPhysicians: [ReferringPhysician]! 
  }

const studyResolvers: GQLStudyTypeResolver<MStudy> = {
  referringPhysicians(study, {}, ctx) {
    return loadReferringPhysicianByIds(ctx, study.referringPhysicianIds);
  },
  studyDescription(study, {}, ctx) {
    if (!study.studyDescriptionId) {
      return null;
    }
    return loadStudyDescriptionById(ctx, study.studyDescriptionId);
  },
};

But thinking about following improvement that I believe could make the result type significantly more useful, don't know how difficult be to add that in.

In this example smartTResult type requires that resolver which is returning GQLStudy would have return all fields (_id, read, studyDescription and referringPhysicians), but in fact since Study type has own resolvers for referringPhysicians and studyDescription ideally it should not require these fields (make them optional) when resolving Study type and require only _id and ready.

That would require to not just look at what type is being returned, but also look into the resolvers if there are some field resolvers.

What do you think, does it make sense?

dangcuuson commented 6 years ago

Hi there,

I see that it would be redundant to resolve some field if you already have defined type resolver of that field's return type.

I think the strategy for this would be if the field is not built-in type (String, Int, Boolean, etc.) then it should be optional. Otherwise required if it's non-null.

We may forget to define resolver for a certain type, but maybe the requireResolverTypes option could help :smiley: What do you think about this strategy?

Additionally, this could result in more types being generated, since the TResult type most likely will be different than the original type (e.g GQLStudy will have studyDescription as required, but GQLStudyResult would have it as optional). However since it's auto-generated it should not be a problem. (I think using conditional type feature in TypeScript 2.8 could help reduce the amount of code being generated, but it may make the types not explicit enough)

jardakotesovec commented 6 years ago

I like that idea to require only built-in type - ideally if that could include all scalars and enums, as I have several scalar types in addition to built-ins. I think this logic would make great coverage in my case. And as you said requireResolverTypes is great addition to that.