feat(graphql): add an extension for type-graphql integration

[raymondfeng]

This PR adds an extension package to provide GraphQL integration using type-graphql.

https://github.com/strongloop/loopback-next/blob/graphql/extensions/graphql/README.md

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[bajtos]

4kLOC of new code, that's a big chunk to review. Who else can help with reviewing this new feature? @raymondfeng @dhmlau

[bajtos]

+1 for depending on a 3rd-party library and not inventing our own solution.

I have few high-level questions:

1. How is this proposal avoiding SELECT N+1 problem? Is type-graphql providing data loaders out of the box or is it up to the application developer to be aware of this problem and work around it? (See e.g. https://itnext.io/what-is-the-n-1-problem-in-graphql-dd4921cb3c1a)

2. A naive implementation of GraphQL layer has usually worse performance than a typical REST API version, because it spends a lot of time on parsing & traversing GraphQL AST. Fastify uses graphql-jit to compile queries into javascript functions that can be further optimized by V8. Depending on the scenario, graphql-jit can be 5x to 30x faster than graphql-js. Does your proposed implementation design allow us to add graphql-jit in a reasonably easy way? Do you perhaps have any other solutions in mind for improving the performance?

3. The biggest benefit I see in a GraphQL layer for LoopBack is the opportunity to leverage our knowledge of models and repository operations and thus make it easier for LB app developers to build standard CRUD-like GraphQL APIs. For example, all GraphQL query resolvers can understand LoopBack's filter language and offer things like limit and skip out of the box. As far as I understand this pull request, it's focusing on the transport side only, i.e. on the mechanics of using type-graphql to provide a generic GraphQL API. I am fine with that as the first increment, but I would like to understand what's the bigger picture and the next steps?

[dhmlau]

4kLOC of new code, that's a big chunk to review. Who else can help with reviewing this new feature? @raymondfeng @dhmlau

Talked to @hacksparrow, he will review this as well. Thanks @hacksparrow!

[raymondfeng]

@bajtos Thank you for the feedback.

I have few high-level questions:
How is this proposal avoiding SELECT N+1 problem? Is type-graphql providing data loaders out of the box or is it up to the application developer to be aware of this problem and work around it? (See e.g. https://itnext.io/what-is-the-n-1-problem-in-graphql-dd4921cb3c1a)

type-graphql does not deal with graph data resolution itself. It simply manages resolver classes which have graphql related decorations. I'm exploring the extension points from type-graphql to see how LoopBack can add values.

• Plug in LoopBack as the IoC container (thus leverages other LoopBack features such as repositories and services)
• Discover/load graphql resolvers

I also take this opportunities to find limitation of type-graphql. For example, it uses a global registry for resolver metadata, which is problematic for us with the ability to start/stop applications.

A naive implementation of GraphQL layer has usually worse performance than a typical REST API version, because it spends a lot of time on parsing & traversing GraphQL AST. Fastify uses graphql-jit to compile queries into javascript functions that can be further optimized by V8. Depending on the scenario, graphql-jit can be 5x to 30x faster than graphql-js. Does your proposed implementation design allow us to add graphql-jit in a reasonably easy way? Do you perhaps have any other solutions in mind for improving the performance?

I'm not there yet. It's my priority to get the right programming model in place 1st.

The biggest benefit I see in a GraphQL layer for LoopBack is the opportunity to leverage our knowledge of models and repository operations and thus make it easier for LB app developers to build standard CRUD-like GraphQL APIs. For example, all GraphQL query resolvers can understand LoopBack's filter language and offer things like limit and skip out of the box. As far as I understand this pull request, it's focusing on the transport side only, i.e. on the mechanics of using type-graphql to provide a generic GraphQL API. I am fine with that as the first increment, but I would like to understand what's the bigger picture and the next steps?

That seems to be possible. We can follow what we did for REST CRUD controllers. For example, generating resolver classes from models and relations.

[bajtos]

I'm exploring the extension points from type-graphql to see how LoopBack can add values.

Makes sense. Unfortunately, this was not clear to me from reading the pull request description and skimming through the docs.

A naive implementation of GraphQL layer has usually worse performance than a typical REST API version, because it spends a lot of time on parsing & traversing GraphQL AST. Fastify uses graphql-jit to compile queries into javascript functions that can be further optimized by V8. Depending on the scenario, graphql-jit can be 5x to 30x faster than graphql-js. Does your proposed implementation design allow us to add graphql-jit in a reasonably easy way? Do you perhaps have any other solutions in mind for improving the performance?

I'm not there yet. It's my priority to get the right programming model in place 1st.

Sure, I am fine to focus on getting right the programming model first. I just want to avoid the situation where we settle on a programming model that will make it very difficult to optimize the underlying implementation for a good performance.

We can follow what we did for REST CRUD controllers. For example, generating resolver classes from models and relations.

+1

I think we should also investigate how to build GraphQL schemas from our ModelDefinition metadata, similarly how we are building JSON/OpenAPI schemas for the REST layer.

BTW, regarding SELECT 1+N problem and DataLoaders, here is a short video explaining both the issue and a possible solution: https://www.youtube.com/watch?v=SSj4BGkIBGo

[MichalLytek]

also take this opportunities to find limitation of type-graphql. For example, it uses a global registry for resolver metadata, which is problematic for us with the ability to start/stop applications.

The 1.0.0-rc.2 has implemented proper schema isolation, so there shouldn't be any problem with restarting app in dev mode (hot reload), apart from a few more MBs allocated due to storing the references to the "new" classes.

Global storage is not a problem, the standard JS Reflect is also global. The issue is with storing the metadata in arrays, not in a WeakMap that would allow the garbage collector to clean up the memory.

If you have any questions, feel free to @ping me 😉

[MichalLytek]

BTW, is the decorators renaming/reexporting intentional?

Are you going to maintain your own docs to not confuse the people with redirecting to typegraphql.com site while importing resolver decorator from @loopback/graphql package?

[bajtos]

I am afraid I don't have bandwidth to keep up with the changes. I am fine to land this as an initial version and then iterate on any improvements needed.

Please review code coverage by tests, I see few files that have lower test coverage than is our current bar:

• resolver resolution - narrow down by key: https://coveralls.io/builds/33198782/source?filename=extensions%2Fgraphql%2Fsrc%2Fgraphql.container.ts#L85
• verify handling of express options: https://coveralls.io/builds/33198782/source?filename=extensions%2Fgraphql%2Fsrc%2Fgraphql.server.ts#L76
• it looks like there is no test loading the graphql example index.ts file: https://coveralls.io/builds/33198782/source?filename=examples%2Fgraphql%2Fsrc%2Findex.ts#L11