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

Discussion: "toAsyncValue" feature proposal #404

Closed
remojansen opened this issue Nov 10, 2016 · 14 comments
Closed

Discussion: "toAsyncValue" feature proposal #404

remojansen opened this issue Nov 10, 2016 · 14 comments

Comments

@remojansen
Copy link
Member

I would like to share my thoughts about a potential breaking change so we can all think about this an ensure that we get it right.

I think this feature is important because I believe that Async / Await is going to be a game changer in all the Node.js libraries and frameworks and this feature will improve it a lot.

The problem

We can use a Provider to deal with classes with an asynchronous initialization process:

container.bind<interfaces.Provider<DbClient>>("Provider<DbClient>")
         .toProvider<DbClient>((context) => {
            return () => {
                return new Promise<DbClient>((resolve, reject) => {

                    // Create instance
                    let dbClient = context.container.get<DbClient>("DbClient");

                    // Open DB connection
                    dbClient.initialize("//connection_string")
                            .then(() => {
                                resolve(dbClient);
                            })
                            .catch((e: Error) => {
                                reject(e);
                            });
                });
            };
        });

This solution is not great because it requires a two step initialization to be done by the users:

// Step 1: Create instance 
let userRepository = container.get<UserRepository>("UserRepository");

// Step 2: Initialize at instance properties using a Provider
userRepository.dbClientProvider()
     .then((dbClient) => { userRepository.dbClient = dbClient; })
     .catch((e: Error) => { console.log(e); });

// At this point the instance is ready to be used
userRepository.search({ country: "Ireland"}).then((irish) => {
  console.log(irish);
});

My first idea...

Now that Async / Await is available (TypeScript 2.1 preview) I though about the possibility of using a async constructors to solve the 2-step initialization problem but I discovered that it is not posible to declare async constructors:

@injectable()
class UserRepository {

    private _dbClient: DbClient;

    // Error: 'async' modifier cannot appear on a constructor declaration.
    public async constructor(
        @inject("Provider<DbClient>") dbClientProvider: interfaces.Provider<DbClient>
    ) {
        this._dbClient = await dbClientProvider();
    }

    public async search(filter: any): Promise<User[]> {
        return await this._dbClient.get("users", filter);
    }

}

If this was allowed we would be able to perform a single-step initialization. However, this single step would always be asynchronous. This means that the Container.prototype.get<T>(serviceIdentifier) method would become asynchronous:

function async doSomething() {

  // Step 1: Create instance 
  let userRepository = await container.get<UserRepository>("UserRepository"); // Async!
  
  // At this point the instance is ready to be used
  let irish = await userRepository.search({ country: "Ireland"});
  
  console.log(irish);
  
}

For users not using async / await would look like:

// Step 1: Create instance 
container.get<UserRepository>("UserRepository").then((userRepository) => {

  // At this point the instance is ready to be used
  userRepository.search({ country: "Ireland"}).then((irish) => {
    console.log(irish);
  });

});

So this solution is not possible but maybe someday it will become possible. Anyway it would require:

  • The Container.prototype.get<T>(serviceIdentifier) method to be asynchronous.
  • The user needs to await the provider and use async constructors (not possibe).

The toAsyncValue binding

It would be nicer if we could use a new kind of binding:

container.bind<DbClient>("DbClient").toAsyncValue((context) => {
    return () => {
        return new Promise<DbClient>((resolve, reject) => {
            let dbClient = new DbClient();
            dbClient.initialize("//connection_string")
                    .then(() => {
                        resolve(dbClient);
                    })
                    .catch((e: Error) => {
                        reject(e);
                    });
        });
    };
});

It looks similar to a Provider binding but it simply expect values to be ready when it is injected even if they require an asynchronous initialization:

@injectable()
class UserRepository {

    private _dbClient: DbClient;

    public constructor(
        @inject("DbClient") dbClient: DbClient
    ) {
        this._dbClient =  dbClient;
    }

    public async search(filter: any): Promise<User[]> {
        return await this._dbClient.get("users", filter);
    }

}

So this solution is possible:

  • The Container.prototype.get<T>(serviceIdentifier) method to be asynchronous.
  • The usage of async constructors is not required.

If we use async / await, the the InversifyJS API will look as follows:

function async doSomething() {

  // Step 1: Create instance 
  let userRepository = await container.get<UserRepository>("UserRepository"); // Async!
  
  // At this point the instance is ready to be used
  let irish = await userRepository.search({ country: "Ireland"});
  
  console.log(irish);
  
}

My main concern about this API is how to continue supporting lazyInjection for React. I'm not sure about being able to lazy inject properties using promises or async / await. I need to investigate this because I don't want to break projects out there. If you have an idea please let me know.

This should be compatible with all the other tools. For example, in Express utils, the router would await the Controllers to be resolved and controller actions would be asynchronous:

@Controller("/api/users")
@injectable()
class UserController {

  private readonly _userRepository: UserRepository;
  
  public constructor(@inject("UserRepository") userRepository: UserRepository) {
    this._userRepository = userRepository;
  }

  @Get("/")
  public async getAll() {
    return await this._userRepository.search({});
  }

  @Get("/search/:filter")
  public async getAll(filter: any) {
    return await this._userRepository.search(filter);
  }
  
}

I like this idea a lot because resolving and awaiting injections is handled by the tools not by the users.

Can you please think about this change and let me know your thoughts:

  • Will your application be able to await the Container.prototype.get<T>(serviceIdentifier) method?
  • How do you feel about this change?
  • Do you have any suggestions / concerns?

If we resolve the lazyInjection problem and everybody is happy, this will be part of the 3.0.0 release.

@lholznagel
Copy link
Contributor

I like that idea, especially because there is one step less when connecting to a database or something similar.

@meticoeus
Copy link

For lazy injection, what about injecting a promise or async getter that resolves to the dependency once it is available? A component waiting on a database connection would probably render a loading state until the data is available regardless of injection method.

@remojansen remojansen changed the title Discussion: toAsyncValue feature proposal Discussion: "toAsyncValue" feature proposal Nov 10, 2016
@remojansen
Copy link
Member Author

Hi @meticoeus at the moment we have the following:

import getDecorators from "inversify-inject-decorators";
import { Kernel, injectable, tagged, named } from "inversify";

let kernel = new Kernel();
let { lazyInject } = getDecorators(kernel);
let TYPES = { Weapon: "Weapon" };

interface Weapon {
    name: string;
    durability: number;
    use(): void;
}

@injectable()
class Sword implements Weapon {
    public name: string;
    public durability: number;
    public constructor() {
        this.durability = 100;
        this.name = "Sword";
    }
    public use() {
        this.durability = this.durability - 10;
    }
}

class Warrior {
    @lazyInject(TYPES.Weapon)
    public weapon: Weapon;
}

kernel.bind<Weapon>(TYPES.Weapon).to(Sword);

let warrior = new Warrior();
console.log(warrior.weapon instanceof Sword); // true

Internally it uses a "get proxy" via Object.defineProperty you can see the details here. I don't think we can await inside the proxy 😢

So I think you are right, the only way would be to inject a promise:

import getDecorators from "inversify-inject-decorators";
import { Kernel, injectable, tagged, named } from "inversify";

let kernel = new Kernel();
let { lazyInject } = getDecorators(kernel);
let TYPES = { Weapon: "Weapon" };

interface Weapon {
    name: string;
    durability: number;
    use(): void;
}

@injectable()
class Sword implements Weapon {
    public name: string;
    public durability: number;
    public constructor() {
        this.durability = 100;
        this.name = "Sword";
    }
    public use() {
        this.durability = this.durability - 10;
    }
}

class Warrior {
    @lazyInject(TYPES.Weapon)
    public weapon: Promise<Weapon>;
    public fight() {
        this.weapon.then((sword) => {
            sword.use();
        });
    }
}

kernel.bind<Weapon>(TYPES.Weapon).to(Sword);

let warrior = new Warrior();

The other option is to inject a function that returns a function. So the warrior can use async and await:

class Warrior {
    @lazyInject(TYPES.Weapon)
    public weapon: () => Promise<Weapon>;
    public async fight() {
        let sword = await this.weapon();
        sword.use();
    }
}

let warrior = new Warrior();
warrior.fight();

What do you guys think?

cc: @otbe @donaldpipowitch you guys are using it with React so it is important to get your feedback.

@rashtao
Copy link

rashtao commented Nov 15, 2016

From a user point of view, I would like to have the possibility to decide case by case whether to inject a Promise or the resolution value of the promise. And that is the point also about using async/await: the consumer can decide to deal with the promise or to await and get the value.
In the constructor parameters annotations, we could have 2 alternatives:

  • @ inject --> injects a promise
  • @ awaitInject --> inject the resolution value of the promise

Furthermore the lazyness concept is a little different to me: it means whether to create the dependency before or to inject a proxy and create the real object as the consumer calls methods on it. So the lazy annotation could be applied to both cases.

@remojansen
Copy link
Member Author

remojansen commented Nov 15, 2016

Hi @rashtao at the moment you can choose between promise or value. All the kinds of injections inject a value or a Factory of the value. If you want a promise we inject a Provider. The provider is a Async Factory.

This means that we can use @Inject("TYPE") to inject a type: value, a factory or a provider.

What is not possible and I would like to make possible is injecting a value that requires an async initialization. In other word be able to await Provider and inject its value. This is not possible at the moment but I don't think we need @awaitInject because you will be able to use:

  • @inject("Provider<T>") allows access to the value via a promise
  • @inject("T") allows direct access to the value without promise

The problem is that to be able to support AsyncValue injections we need the library to be async by default. So you will be able to choose if you want to inject a promise or a value but you will always have to await to get that promise or that value:

container.bind<Warrior>("Warrior").to(Ninja);

// we have to await but it is synchronous
let ninja = await container.get<Warrior>("Warrior"); 
// ninja is a value
container.bind<DbClient>("DbClient").toAsyncValue((context) => {
    return () => {
        return new Promise<DbClient>((resolve, reject) => {
            // ...
        });
    };
});

// we have to await because is asynchronous
let dbClient = await container.get<DbClient>("DbClient");
// dbClient is a value
container.bind<interfaces.Provider<Katana>>("Provider<Katana>").toProvider<Katana>((context) => {
    return () => {
        return new Promise<Katana>((resolve) => {
            let katana = context.container.get<Katana>("Katana");
            resolve(katana);
        });
    };
});

// we have to await but it is synchronous
let katanaProvider= await container.get<interfaces.Provider<Katana>>("Provider<Katana>");
// katanaProvider is a Promise

Note that we await only at the composition root not in every level of the composition tree. We also await in the levels in which a async value is injected.

@rashtao
Copy link

rashtao commented Nov 15, 2016

thanks for the clarification!
having a flexible @inject would be nice... Anyways from the consumer point of view: @inject("Provider<T>") would be the same of @inject("Promise<T>"), right?

@remojansen
Copy link
Member Author

Not 100% the same:

  • @inject("Provider<T>") injects a function that returns a promise () => Promise<T>
  • @inject("Promise<T>") injects a promise Promise<T>

@buehler
Copy link

buehler commented Nov 16, 2016

Hey @remojansen

My 2 cents to this matter :-)

First of all: async constructor are not gonna happen in my opinion. The simple reason is: no language I know (maybe not that much) does support it. Since TypeScript is using many core concepts of C#, the way to go (as in Java or similar languages) looks like this (Possible with TypeScript 2):

class Foobar {
    private constructor(private dbInstance: any) {}
    public static async create(/* whatever */): Foobar {
        let db = await getMyDbInstance();
        return new Foobar(db);
    }
}

So much for the constructors ;-)

So as I understand this discussion, it's not about some critical missing part, it's about convenience? So you actually want to inject a previously awaited inject?

Problem is, that if you want to achieve that, you need to await all the things before you create the instance.

I could imagine the following scenarios:

class Foobar {
    private async getDb(): Promise<Db> {
        return await container.get; //...
    }
}

or use a factory (async factory) with the code at the beginning of this post (the async create method) to inject the value.

Another possibility could be a container.get method, which returns the result immediately but with Promises injected (or factories / provider) and a container.getAsync method, that can be awaited but awaits all Promises that are injected.

Do I understand the question?

Cheers

@remojansen
Copy link
Member Author

Hi @buehler thanks a lot for your feedback. Having container.get and container.getAsync is something that I have actually think about. I see two problems:

  • Provider<T> is an alias for (...args: any[]) => Promise<T> and it can take some arguments provided by the user not the container. So provider should always be injected as a factory that returns a promise never as the value returned by the promise.
  • AsyncValue injection does not allow user provided args () => Promise<T> this means that we could inject it as a value when container.getAsync is used or as a promise when container.get is used. But this makes the code hard to understand because your class may be annotated as @inject("DbClient") and the reality is that DbClient will sometimes be a promise and sometimes be a value based on the container method invoked.

@remojansen
Copy link
Member Author

remojansen commented Nov 16, 2016

Maybe it is not worth it to introduce async value injection?

At the moment we can do:

class UserRepository { 

    private _db: DbClient;
    private _dbProvider: Provider<DbClient>;

    // STEP 1
    public constructor(
        @inject("Provider<DbClient>") provider: Provider<DbClient> // Provider injection 
    ) { 
        this._dbProvider = provider;
    }

    // STEP 2
    private async init() {
        if (this._db) return this._db;
        this._db = await this._dbProvider();
        return Promise.resolve(this._db);
    }

    public async getUser(): Promise<Users[]>{
        let db = await this.init();
        return db.collections.user.get({});
    }

    public async deletetUser(id: number): Promise<boolean>{
        let db = await this.init();
        return db.collections.user.delete({ id: id });
    }

}

It requires a two-step initialization but the code is not bad.

I wanted to allow one step initialization:

class UserRepository { 

    private _db: DbClient;

    // STEP 1
    public constructor(
        @inject("DbClient") db: DbClient // Async value injection
    ) { 
        this._db = db;
    }

    public async getUser(): Promise<Users[]>{
        return this._db.collections.user.get({});
    }

    public async deletetUser(id: number): Promise<boolean>{
        return this._db.collections.user.delete({ id: id });
    }

}

But it requires the whole framework to become async. Maybe the price to pay is too high for just one small improvement?

@buehler
Copy link

buehler commented Nov 16, 2016

I like the first code ;-) It's neat and understandable. Maybe the price is too high. If the whole framework needs to get async I don't know if the features are that great at the moment.

But maybe for features in the future, there will be the need to make the framework async, but I'd wait for that to happen.

A possible extension to make the code cleaner could be an extension to the Provider:

class Provider<T>{
    public readonly value: T;

    public get someFancyNameForProvideValue(): Promise<T> {
        return this.value !== undefined ? Promise.resolve(this.value): this.provide();
    }

    public provide(): Promise<T> {
         /* exec provider function */
         this.value = 42; // save the result of the provider function
    }
}

// then you could make either:

class UserRepository { 

    // STEP 1
    public constructor(
        @inject("Provider<DbClient>") private provider: Provider<DbClient> // Provider injection 
    ) {}

    public async getUser(): Promise<Users[]>{
        let db = this.provider.value || await this.provider.provide();
        return db.collections.user.get({});
    }
}

// or (would be cool too, with the fancy getter):

class UserRepository { 

    // STEP 1
    public constructor(
        @inject("Provider<DbClient>") private provider: Provider<DbClient> // Provider injection 
    ) {}

    public async getUser(): Promise<Users[]>{
        let db = await this.provider.someFancyNameForProvideValue;
        return db.collections.user.get({});
    }
}

The idea would be to extend the provider so that it is a class that has a provide function. (Maybe)
So you could "save" the init function and just await the provider function that returns the value if initialized, or executes the function and saves the value.

@remojansen
Copy link
Member Author

I'm going to investigate the @buehler suggestion.toAsyncValue is not going to happen because it is probably too early to force people to use async / await. Thanks a lot for your feedback :)

@buehler
Copy link

buehler commented Nov 18, 2016

No worries mate :-)

@remojansen
Copy link
Member Author

I have created #418 I'm closing this issue 😄

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

No branches or pull requests

5 participants