0.4.0

[pleerock]

Im going to release next 0.4.0 version. There are a lot of changes and improvements, and breaking changes too. Last breaking change I made today - I've changed ValidationError signature. Before releasing I wanted to ask you guys @zakhenry @DominicBoettger if there is something you think is wrong in this release, or something should be added/changed before this release.

Thank you

[zakhenry]

Looks good to me, the upgrade for me to 0.4.0 was relatively straight forward, only complexity was in dealing with the new container

[pleerock]

really? it was only one-line change

[zakhenry]

From memory the difference was that now everything is retrieved from the container (previously just instantiated), and I was providing my own container, but it had no reference to Validator for example. So I had to extend my container to fall back to instantiation.

[zakhenry]

The only way to make that simpler would be do do the following:

Change

export function useContainer(iocContainer: { get(someClass: any): any }) {
    container = iocContainer;
}

To

export function useContainer(iocContainer: { get(someClass: any): any }) {
    const originalContainer = container;
    container = {
        get: (someClass: any) => {
            try {
                return iocContainer.get(someClass);
            } catch () {
                return originalContainer.get(someClass);
            }
        }
    }
}

(The above is a crude proof of concept to demonstrate the point, it should be a cleaner implementation where the original container is a class that can have a container passed to it to try first)

[zakhenry]

Actually you could probably use my implementation directly:

interface InjectableClass<T> {
    new (...args: any[]): T
}

interface IOCContainer {
    get(someClass: any): any
}

class Container implements IOCContainer {

    protected externalInjector: IOCContainer;

    protected instances: WeakMap<InjectableClass<any>, any> = new WeakMap();

    /**
     * Get the instance of a class when there is no dependency
     * @param staticClass
     * @returns {any}
     */
    public noDependencyGet<T>(staticClass: InjectableClass<T>): T {

        if (!this.instances.has(staticClass)) {
            this.instances.set(staticClass, new staticClass());
        }

        return this.instances.get(staticClass);
    }

    public get<T>(staticClass: InjectableClass<T>): T {
        if (!this.externalInjector) {
            return this.noDependencyGet(staticClass);
        }

        try {
            return this.externalInjector.get(staticClass);
        } catch (e) {
            return this.noDependencyGet(staticClass);
        }
    }

    public setContainer(iocContainer: IOCContainer): this {
        this.externalInjector = iocContainer;
        return this;
    }
}

[pleerock]

as I understood, your container cannot simply return instance of the class it received, thats why fallback to original container is required. But I thought most containers working like this: you send them refrenece to the class / token and they return you instance of this class, without complaining

[zakhenry]

Yep thats the problem, I guess not all containers do that - I'm just using the Angular 2 injector, so probably a fairly common use case. The Angular 2 injector stores a reference of all provided Tokens, and if there is no provider for that token it throws a NoProvider exception

[pleerock]

I guess it will not give you instance of the class, until you explicitly "provide" it?

[zakhenry]

Correct, and providing tokens is done when the injector instance is created.

For example:

import { ReflectiveInjector } from  '@angular/core';
const providers = ReflectiveInjector.resolve([
    {provide: Validator, useClass: MyValidator}
]);
const injector  = ReflectiveInjector.fromResolvedProviders(providers);

[pleerock]

got it.

the reason why angular's container works differently then others, I think mostly because it was designed to work with tree-like components. And this design works fine with tree-like components when you can provide any instance on any level of the tree - I like this concept when designing view components

[zakhenry]

Yea, also the Angular container can handle hierarchical dependencies - the injected class can be dependent on other injected classes.

I actually have a unit test showing this working with a custom validator having external dependencies, see https://github.com/ubiquits/ubiquits/blob/master/src/common/stores/store.spec.ts#L16-71

[pleerock]

so, about fallback, probably I need not only catch an exceptions, but also check for null/undefined references?

get: (someClass: any) => {
            try {
               const instance = iocContainer.get(someClass);
               if (instance)
                   return instance;
            } catch (error) {
            }
            return originalContainer.get(someClass);
        }

and what shall I do with error? is it a really good to catch errors and completely ignore them? can be a situation when there is some another error, that you user should be aware of?

[zakhenry]

Yea thats probably a good idea to check if defined. Good point on the error, maybe if a setContainerFallbackCondition((error:Error):boolean):this could be added?

try {
    const instance = iocContainer.get(someClass);
    if (instance) {
        return instance;
    }
} catch (error) {
    if (this.containerFallbackCondition && !this.containerFallbackCondition(error)) {
        throw error;
    }
}
return originalContainer.get(someClass);

[pleerock]

this makes it complicated, I think this kind of logic should be resolved on client side. Maybe something simple like this:

useContainer(container, { fallback: true, fallbackOnErrors: true });

interface UseContainerOptions {

    /**
     * If set to true, then default container will be used in the case if given container haven't returned anything.
     */
    fallback?: boolean;

    /**
     * If set to true, then default container will be used in the case if given container thrown an exception.
     */
    fallbackOnErrors?: boolean;

}

[zakhenry]

Maybe if fallbackOnErrors could be fallbackOnErrors?: boolean | (error:Error):boolean ? Best of both worlds?

[pleerock]

It will be same as

 const instance = iocContainer.get(someClass);
    if (instance) {
        return instance;
    }
  return originalContainer.get(someClass);

e.g. without try/catch. Without fallback: true it will work like now, .e.g without any fallback at all.

Maybe if fallbackOnErrors could be fallbackOnErrors?: boolean | (error:Error):boolean ? Best of both worlds?

I think it overcomplicates it - "handing error based on some function that returns true or false for this error" - seems very specific feature, and if needed user should handle it by themself, by passing a custom container that handle it.

[zakhenry]

Yea sorry I misread the comments in the interface, understood now

[pleerock]

here is how it will look like: #20

[pleerock]

released https://github.com/pleerock/class-validator/releases/tag/0.4.0