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

[discuss] Type-graphql types #781

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

NickBolles
Copy link
Contributor

A quick implementation of type-graphql for graphql-api.

I'm wondering if we want to create a new package for type-graphql by itself. Right now I am not exporting the type-graphql types from the root package because that will for users to have type-graphql installed even if they aren't using it.

Looking for input on if this is alright, or create a new package.

upgrade graphql-codegen to 1.7.x and use new decoratorName config  from dotansimha/graphql-code-generator#2448
@NickBolles
Copy link
Contributor Author

NickBolles commented Sep 4, 2019

Alright I think this is ready, pending circleCI check completing. Looks like a memory issue.

Main main remaining questions are:

  1. Is this the right way to release this? As a file in the lib folder that isn't exported by default? The main reason I did this is because this way all users aren't required to have type-graphql installed. They can just import it if they need to.
  2. Is having this in the graphql-api an okay approach? should I just create a new package that builds just the type-graphql definitions? then we can have type-graphql as a dependency and have it be the default export for the package. It also would increase visibility for the package.
  3. Is always exporting interfaces okay? should we have different modes? My use case requires interfaces because I want to extend a different base class in my derived types. So using an interface types allows me to implement it, stub the properties and get the GraphQL types for free.

@TimMikeladze
Copy link
Member

@NickBolles Thank you so much for doing this work, I just started using TypeGraphQL so this PR comes at a perfect time.

  1. I think this is fine.

  2. Currently the graphql-api is implemented using the https://github.com/Urigo/graphql-modules package. A graphql module is exported. All the typedefs are defined as gql strings, with some additional logic in place for supporting the extends graphql syntax and creating a graphql module.

Ultimately all the the graphql-api needs to do is export is a set of resolvers, typedefs and an executable schema. There can be two separate packages which consume these, in our case it would be an additional package which creates and exports a graphql module and then another package which reexports the type-graphql definitions.

However I think we may need to reexamine our overall approach in graphql-api. We could have it rely entirely on type-graphql to define typedefs, resolvers and exporting an executable schema. With an additional packages which wraps those up as a graphql module, or create typeorm entities, etc. @pradel @davidyaha What are your thoughts?

Our goal should be to provide consumers as seamless integration with the accounts graphql schema as possible. With clear guidelines on how to extend the accounts functionality, whether that be on a database level with custom entities or on a graphql level, in a monolithic or federated approach.

  1. I think an option for exporting interfaces would be ideal.

@NickBolles
Copy link
Contributor Author

@TimMikeladze I don't have a ton of time to respond, and there's a lot to digest, but I think I'm continuing to lean more towards a type-graphql package that's seperate from graphql-api. Whether graphql-api uses type-graphql or the resolvers are centralized can be a different discussion probably. But I'm running into some issues with conflicting types between graphql-api and type-graphql

Especially when you through nestjs and an ORM (like typegoose) into the mix so that you can't just extend the base type-graphql type.

@NickBolles NickBolles closed this Sep 5, 2019
@NickBolles NickBolles reopened this Sep 5, 2019
@NickBolles
Copy link
Contributor Author

Oops, didn't mean to close

@TimMikeladze
Copy link
Member

@NickBolles Sounds good, I think creating a separate type-graphql package and seeing how it is implemented will help us decide what the future of graphql-api is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants