prismake / typegql

Create GraphQL schema with TypeScript classes.
https://prismake.github.io/typegql/
MIT License
423 stars 21 forks source link

Suggestions around Schema instantiation #20

Closed lorefnon closed 6 years ago

lorefnon commented 6 years ago

I have two suggestions around the way root resolvers are invoked :

  1. I found it very non intuitive that the root schema classes are never instantiated. So in a sense, the class exists just to serve as a container to be annotated but that prototype is not used. This is not what most users would expect and I'd expect resolver methods to be invoked in the class context. This PR addresses that.

  2. It may not be ideal to have all root resolvers/mutations in a single schema. I do realize it is possible to use utilities like Apollo's graphql schema stitching to merge multiple graphql APIs but it seems like an unnecessary overhead to facilitate modularity when all implementation resides in the same project.

I do realize that the above two are separate concerns, but they required modifications in same code paths so I conflated them in a single PR. If you'd like I can split them out in separate PRs.

I have so far only updated the code and tests, but I'll be happy to update the docs/examples as well if you find the suggested changes agreeable.

AFAIK there is nothing breaking here.

codecov[bot] commented 6 years ago

Codecov Report

Merging #20 into master will increase coverage by 0.11%. The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   87.33%   87.45%   +0.11%     
==========================================
  Files          61       61              
  Lines        1058     1068      +10     
  Branches      181      183       +2     
==========================================
+ Hits          924      934      +10     
  Misses        133      133              
  Partials        1        1
Impacted Files Coverage Δ
src/domains/schema/registry.ts 100% <ø> (ø) :arrow_up:
src/domains/schema/compiler.ts 93.54% <100%> (+1.24%) :arrow_up:
src/domains/field/compiler/resolver.ts 91.56% <88.88%> (+0.54%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 9bed763...01289f9. Read the comment docs.

pie6k commented 6 years ago

Thanks @lorefnon . Thanks for this PR. I've also seen need for improving api for compiling schema. Right now, as you've said it forces 'central' place for all queries and mutations.

As I see in your code, new api would allow things like

const schema = compileSchema([
  FooSchema,
  BarSchema
]);

I like idea, but I would iterate over it a bit more.

What do you think about concept of @SchemaRoot (that's kind of alias for current @Schema) and then you could compile schema like:

const schema = compileSchema({
  roots: [RootA, RootB, RootC],
});

each of those roots can have @Query or @Mutation fields. That should solve the problem of scalability. Also, introducing object as parameter of compileSchema makes it future-proof.

I'd also keep concept of abstract class that 'joins' root fields and give name to each of them. It might be good idea to emphasize that those should be abstract classes.

When Typescript will allow decorating functions - such class could become useless. I don't have strong opinions on this, however.

Let me know.

pie6k commented 6 years ago

I've added another PR showing my iteration over it: https://github.com/prismake/typegql/pull/21

I was thinking about modifying your PR, but because of instances aspect you've added, it'd be more complex.

pie6k commented 6 years ago

I'll add concept of instance of schemaRoot, but I've merged different approach to constructing schema from schemaRoots, so it's too different now.