Expose default container instances to external containers

[fernandolguevara]

Expose default container instances to external containers when useContainer is invoke

[fernandolguevara]

@19majkel94 review please =)

[MichalLytek]

What is the purpose of that feature? Why you can't just use TypeDI along with class-validator?

[fernandolguevara]

@19majkel94 of course i can use type di or another IoC container, but this feature is to expose the internal configuration of class-validator.

run the next example

import { validate, useContainer, IsNotEmpty } from "../../src/index";

export class Customer {
    @IsNotEmpty()
    name: string;
}

const customer = new Customer();

validate(customer).then(result => {
    console.log('1 val', result);
});

const defaultContainer = new (class {
    private instances: any[] = [];
    get<T>(someClass: { new(...args: any[]): T }): T {
        let instance = this.instances.find(instance => instance.type === someClass);
        if (!instance) {
            instance = { type: someClass, object: new someClass() };
            this.instances.push(instance);
        }

        return instance.object;
    }

    set(instances: any[]): void {
        this.instances = [...instances, this.instances];
    }
})();

useContainer(defaultContainer, {
    registerExistingInstances: (instances) => {
        // defaultContainer.set(instances); // un-comment  
    }
});

validate(customer).then(result => {
    console.log('2 val', result);
});

if you import a class decorated before you call useContainer function, an instance of MetadataStorage is registered in the default container with the configuration of imported class, so its posible that in our systems exists two instances of MetadataStorage with partial configurations one created by the internal container of the library and one created by type di.

result with commented line

result with un-commented line

possible solution to #261

I think that same problem also exists in routing-controllers

[DavidBabel]

I like it :)

[MichalLytek]

if you import a class decorated before you call useContainer function, an instance of MetadataStorage is registered in the default container with the configuration of imported class, so its posible that in our systems exists two instances of MetadataStorage with partial configurations one created by the internal container of the library and one created by type di.

So @fernandolguevara maybe just do this:

import "reflect-metadata"
import { Container } from "typedi"
import { useContainer } from "class-validator"
useContainer(Container)

import MyType from "./myFile"
// etc.

Default container is an internal fallback. It shouldn't be exposed, it's not designed as an universal DI container. registerExistingInstances is only complicating things. And metadata shouldn't be stored in container - in TypeORM or TypeGraphQL we store it in nodejs global scope.

[fernandolguevara]

@19majkel94 solving the issue by ordering the imports in a specific way, its not a solution, i'm not agree with that.

in the pr, the default container it's not exposed as universal di, only its instances.

maybe the solution is to store the metadata in global scope...

I can make that change if you prefer and make another PR #265

[vlapo]

@satanTime could you please check this PR and #265. I would like to hear your opinion on these changes.

[satanTime]

Sure, I'll do that tonight.

[satanTime]

Hi @vlapo,

Long story short: it has been solved by b57fef4.

This issue is about the similar problem when we want to get access to the original metadata definition when we have different DI providers / contexts.
The author says:

if you import a class decorated before you call useContainer function, an instance of MetadataStorage is registered in the default container...

Because now MetadataStorage is shared among whole applications through getMetadataStorage() and the global.classValidatorMetadataStorage variable, there is no need in the PR and it can be simply closed.

Let me know if you need more input from me.

[vlapo]

Thank you @satanTime. I agree with you. Just want to know your opinion as you modify DI mechanism lately.

Feel free to reopen in case someone has different opinion.

[fernandolguevara]

@satanTime @vlapo
Hi guys, next time I hope you accept the contributions of your users, you could have accepted my pull request instead of copying the code that was in the PR #265 - b57fef4.

[satanTime]

Hi @fernandolguevara,

I think there's a misunderstanding, your PR was closed without copying. The issue was solved in a bit different way, like typeorm and routing-controllers do it.

Feel free to ping me if the fix doesn't work for you.
Then we could contribute to typeorm, routing-controllers and here with a better solution.

[fernandolguevara]

@satanTime do you see the code on the PR #265 ? i think is the same code that yout commit here b57fef4 👎🏽

[satanTime]

Ah, now I see what you mean. I can assure you my PR was copy of routing controllers implementation from here typestack/routing-controllers@c263928.

[vlapo]

@fernandolguevara Nobody copied your solution. I think @satanTime only had same idea (inspired by typeorm, routing-controller).

It was not my intention to accept his PR before yours. It was just my fault that I did not notice the similarity of the PRs. I'll be more careful next time.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.