typestack / typedi

Simple yet powerful dependency injection tool for JavaScript and TypeScript.
https://docs.typestack.community/typedi/v/develop/01-getting-started
MIT License
4.05k stars 168 forks source link

feat: support for asynchronous factory functions #84

Open slavafomin opened 5 years ago

slavafomin commented 5 years ago

Hello!

Consider a case where you have an asynchronous factory function: for example you need to make a request or read some file from the filesystem before creating an instance of the required class.

Right now, TypeDI, if supplied with the async factory function, will add the promise object to the container instead of resolving it beforehand.

I think it's a valid use case, but in order to implement it, we will probably need to change all container's call signatures to async ones, which is a very drastic change in my opinion. However, this enables some very interesting functionality.

The alternative would be to just resolve those factory functions manually during the app initialization and register instances with the container, which could be non-optimal, because we don't know if an instance some class will be actually used during program execution (sacrificing lazy-loading).

What do you think?


The manual approach looks like this:

  const dataServiceFactory = Container.get(DataServiceFactory);
  const dataService = await dataServiceFactory.create();

  Container.set({
    type: DataService,
    value: dataService
  });

Which is somewhat cumbersome and non-automatic.

dbartholomae commented 5 years ago

I would be interested in something like this as well: typeorm needs to first async connect to a db before repositories can be injected, and the DI being all sync forces me to use non-DI before. I am aware though that there are discussions that DI should only be synchronous, and I am no expert on that matter.

StevenLangbroek commented 4 years ago

Hey folks! I was wondering if this is under consideration? I think this is a very common usecase (e.g., database connections, oauth providers that open & maintain a connection), and they all need to be lifted outside of your IoC in order to be feasible.

NoNameProvided commented 4 years ago

This has a pending PR in #126.

NoNameProvided commented 4 years ago

We need to discuss how to surface this API in a useable way. My relevant comment from the PR:

I'm not sure that I understand your point, could you elaborate on this please? ... If you are not using real async calls to initialize your services than all promises will be resolved synchronously.

In the above comment, you mentioned that we should change the entire TypeDI API to async, which means everything needs to return a Promise.

Creating promises is not cheap, in fact, there is like a 1:10 performance hit with async functions. If someone requests an instance from TypeDI in a loop or for example for every request on a web server, that will definitely add up. Once we started down this path, the user will have to create extra sync functions just to be able to await the Service, so ther performance hit will be rather depth of functions x 10 which is huge.

A very bacic perf test:

let counter = 0;

async function asyncTest() { return 1 + 1; }
function test() { return 1 + 1; }

// call both of them a lot of time
// counter = asyncTest() vs counter = test()

image

weoreference commented 3 years ago

so what now? no solution?

dilame commented 3 years ago

We need to discuss how to surface this API in a useable way

I think there is a very simple solution, but it brings some magic: TypeDI should check if some of the providers return Promise - then whole container.get() should return a Promise, otherwise, it just returns an instance. For old users nothing will change, all the new users should always use await container.get(). await keyword works nice with primitive values and brings no overhead, but if there is a Promise - it will handle it.

Another option is simply add an option await container.get(SomeClass, {async: true}).

What do you think about it?

NoNameProvided commented 3 years ago

I think there is a very simple solution, but it brings some magic: TypeDI should check if some of the providers return Promise - then the whole container.get() should return a Promise, otherwise, it just returns an instance.

That is not UX friendly, to do this we need to mark the return type T | Promise<T> so TypeScript will force users to check their return value always. Currently, my plan for this to wait until a few more feature I am planning is implemented, eg: proper container inheritance and add this as last big feature before 1.0.

I don't find this a critical feature anyway as it is easily fixed via awaiting your setup call in your service on the first line in your functions. Example:

class MyAsyncClass() {
  private readonly initialized!: Promise<unkonwn>;

  constructor() {
    this.initalized = this.initalize();
  }

  private initialize(): Promise<unkown> {
    // your asnyc setup logic
    // return a promise what resovles when class is initialized
  }

  public doSomeStuff(): Promise<unkown> {
    await this.initalized;
    // do your stuff here, this code will run only if initalized has finished running
    // or the function will throw an error if initalized is a rejected promise rejected
  }
}

This way your class created immediately in a sync fashion, but only start processing requests after the initialize call is finished running.

intellix commented 3 years ago

Was hoping for this myself but then didn't need anything special in the end anyway. I'm using Google Cloud Secret Manager and wanted to either use process.env.SOME_SECRET or fetch it async if it's a particular format.

Before:

function configureProviders() {
  Container.set(SEQUELIZE_CONFIG, {
    password: process.env.POSTGRES_PASSWORD,
  });
}

async function main() {
  configureProviders();
  server.listen(PORT);
}

After:

async function resolveEnv(key: string) {
  const value = process.env[key];
  return value && value.startsWith('projects/') && value.includes('/secrets/')
    ? Container.get(GoogleSecretManager).getSecretData(value)
    : value;
}

async function configureProviders() {
  Container.set(SEQUELIZE_CONFIG, {
    password: await resolveEnv('POSTGRES_PASSWORD'),
  });
}

async function main() {
  await configureProviders();
  server.listen(PORT);
}

Now my secrets support either plain text or secret references in ENV and I didn't need anything magical

kgkg commented 2 years ago

Maybe instead of changing get it would be much more user-friendly to add resolve method which will await for async factories to finish and always returns Promise?

Something like

const service = await Container.resolve(MyService);