MichalLytek / type-graphql

Create GraphQL schema and resolvers with TypeScript, using classes and decorators!
https://typegraphql.com
MIT License
8.04k stars 678 forks source link

Async container support #399

Closed backbone87 closed 4 years ago

backbone87 commented 5 years ago

Is your feature request related to a problem? Please describe. I want to use a container that returns promises for the instances retrieved from the container. Some services like DB connections require async construction.

Describe the solution you'd like The easiest way to support this, is to just await the result of the ContainerType.get method. There is a minor BC break here, where return values of sync containers that are thenable, would be awaited, too.

Describe alternatives you've considered

MichalLytek commented 5 years ago

Async containers makes no sense in DI world as class constructors are sync only, so you can't await new Foo() to receive new instance.

Some services like DB connections require async construction.

In all Node.js DI system there are only async providers and they await them before starting resolving dependencies.

So all you need to do is to wait for the DB connection and then put the connection object in container, all before creating the server:

async function main() {
  const connection = await createConnection(opts1);
  Container.set("connection", connection);

  // connection is resolved before, we can synchronously construct a class
  const service = Container.get(MyService);
  const schema = await buildSchema(opts2);
}

Convert an async container to sync one, by waiting for all services to be constructed, which is possibly expensive, especially on per request containers.

Node.js is single threaded, you don't need to connect to DB for each request.

backbone87 commented 5 years ago

Async containers makes no sense in DI world as class constructors are sync only

Containers are by no means limited to class constructors. A dependency can be anything (primitive, object, function, whatever) and the creation of the such a dependency can be sync or async.

Node.js is single threaded.

The fact that node is single threaded has nothing to do with this. When i have a container which contains async factories/providers in order to make the container sync i need to create and wait for every single dependency, even if i dont need all deps to handle a specific request.

you don't need to connect to DB for each request

That depends on how you use your DB. if you want to use transactions, you need to maintain a connection pool and a single DB connection can only be used by 1 (transactional) request at a time. so you need a way to wait for the next free connection.

MichalLytek commented 5 years ago

if you want to use transactions, you need to maintain a connection pool and a single DB connection can only be used by 1 (transactional) request at a time.

So create a connection pool ahead of time and then just assign them when someone request a query in a transaction?

so you need a way to wait for the next free connection.

Why you need to await for the whole service instead of awaiting on connection.createTransaction() or connection.query?

It's your only use case that is incompatible with all DI in JS that I know (TypeDI, NestJS/Angular DI, Awlix, Inversify, etc.). You need to convince me, show code samples, async container libs or something 😉

raymondfeng commented 4 years ago

Async containers makes no sense in DI world as class constructors are sync only, so you can't await new Foo() to receive new instance.

This is NOT true at all :-). With DI, application code usually don't call new Foo() any more. Even the constructor itself is sync, it's perfect fine to have some logic to prepare the args for the constructor asynchronously. For example:

// Create an IoC container
const requestCtx = new Context('request');

// Bind the logger class
requestCtx.bind('logger').toClass(RequestLogger);

// Resolve the logger instance asynchronously as the logger may depend on other information that have to be resolved asynchronously - such as reading the logging level from a file
const myService = await requestCtx.get<MyService>('my-service');

FYI, we (https://github.com/strongloop/loopback-next) build an async IoC/DI container. See more details at https://loopback.io/doc/en/lb4/Dependency-injection.html

I'm interested in integrating @loopback/context as a DI container for type-graphql. What contract do we need to follow?

MichalLytek commented 4 years ago

it's perfect fine to have some logic to prepare the args for the constructor asynchronously

Example, example, example - I have to see a use case that will convince me. For now it's just only about doing the work before getting from container, like create a connection pool to use instead of connecting to db on container.get.

the logger may depend on other information that have to be resolved asynchronously - such as reading the logging level from a file

In this example:

const requestCtx = new Context('request');

const logLevel = await getLogLevelFromConfigFile();
requestCtx.bind('logLevel').toValue(logLevel);

requestCtx.bind('logger').toClass(RequestLogger);

const myService = requestCtx.get<MyService>('my-service');

What contract do we need to follow?

https://github.com/MichalLytek/type-graphql/blob/445ae0a2d4c9d5581be21d84bf1002a76e622829/src/utils/container.ts#L5-L7

Provide an object with a get method that will receive a class constructor and expect to return an instance of it.

raymondfeng commented 4 years ago

For the interface,

export interface ContainerType { 
   get(someClass: any, resolverData: ResolverData<any>): any; 
}

Would it allow get to return Promise<...>?

raymondfeng commented 4 years ago

Here is an example that requires the Ioc/DI to be async:

import * as fs from 'fx-extra';
import {inject} from '@loopback/context';

// MyLogger depends on the logging level
export class MyLogger {
  // Dependency injection for logging level
  @inject('logging.level') private level: string;

   log(msg: string) {
     if (this.level === 'info') { ... }
   }
}

// logging level is resolved asynchronously from a file so that it always gets the latest value
ctx.bind('logging.level').toDynamicValue(async () => {
  const config = await fs.readJson('logging.config.json');
  return config.level;
});

// Register MyLogger 
ctx.bind('myLogger').toClass(MyLogger);

// Now get an instance of MyLogger
const logger: MyLogger = await ctx.get('myLogger');
MichalLytek commented 4 years ago

Would it allow get to return Promise<...>?

For now not - I'm not awaiting the returned value so it doesn't promises.

Here is an example that requires the Ioc/DI to be async:

This is only the example implementation. I need example use case, the problem that you have to solve with async container.

What is the real life use case for dynamic logging level changing? AFAIK it will affect only newly created services, so if for some unknown reason admin connect to the server by SSH and modify the JSON file content, some of the old services will still log on the old level, some not... it makes no sense at all 😄

raymondfeng commented 4 years ago

What is the real life use case for dynamic logging level changing? AFAIK it will affect only newly created services, so if for some unknown reason admin connect to the server by SSH and modify the JSON file content, some of the old services will still log on the old level, some not... it makes no sense at all

It's just over-simplified use case. In reality, the logging level can be changed by admins by a UI - say - for troubleshooting purpose, from info to debug.

In LoopBack, we can inject a getter function too so that the logging level can be read per request. See https://loopback.io/doc/en/lb4/Decorators_inject.html.

MichalLytek commented 4 years ago

In LoopBack, we can inject a getter function too so that the logging level can be read per request.

So in the logger case, you can just use the getter feature and do await this.loggingLevelGetter(); in the method, or even a loggerGetter as you need to do some async stuff in the runtime 😉

MichalLytek commented 4 years ago

I have found a real-world use case: https://github.com/MichalLytek/typegraphql-nestjs/issues/2

So moving that to 1.0.0 milestone and working on a proper support 💪

MichalLytek commented 4 years ago

Closing via 012a561 🔒