whenTargetIsDefault not working in combination with optional

[rainerschuster]

If a property is declared as @optional and the types are bound with whenTargetIsDefault the property is undefined.

Expected Behavior

The following example should inject Katana for the optional katana field.

import { injectable, inject, optional, Container, named } from "inversify";
import "reflect-metadata";

let TYPES = {
  Weapon: "Weapon"
};

let TAG = {
  throwable: "throwable"
};

interface Weapon {
  name: string;
}

@injectable()
class Katana implements Weapon {
    public name: string;
    public constructor() {
        this.name = "Katana";
    }
}

@injectable()
class Shuriken implements Weapon {
    public name: string;
    public constructor() {
        this.name = "Shuriken";
    }
}

@injectable()
class Ninja {
    public name: string;
    public katana: Katana;
    public shuriken: Shuriken;
    public constructor(
        @inject(TYPES.Weapon) @optional() katana: Weapon, // Optional!
        @inject(TYPES.Weapon) @named(TAG.throwable) shuriken: Weapon
    ) {
        this.name = "Ninja";
        this.katana = katana;
        this.shuriken = shuriken;
    }
}

let container = new Container();

container.bind<Weapon>(TYPES.Weapon).to(Shuriken).whenTargetNamed(TAG.throwable);
container.bind<Weapon>(TYPES.Weapon).to(Katana).whenTargetIsDefault();

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

let ninja = container.get<Ninja>("Ninja");
console.log("ninja.name", ninja.name);
console.log("ninja.katana", ninja.katana);
console.log("ninja.shuriken", ninja.shuriken);

Current Behavior

katana is undefined. If the decorator optional is removed it works as expected.

Possible Solution

use container.get(TYPES.Weapon);

Steps to Reproduce (for bugs)

Your Environment

• "inversify": "^5.0.1"
• "reflect-metadata": "^0.1.13"

[notaphplover]

Hi @rainerschuster I'll have a look. Thank you for your report

[notaphplover]

Hi @rainerschuster it makes sense to me to update the actual behavior to cover your expectations. What do you think @PodaruDragos , @dcavanagh?

[PodaruDragos]

hi @notaphplover isn't the behavior correct ? https://github.com/inversify/InversifyJS/issues/411#issuecomment-264651698

[notaphplover]

Hi @PodaruDragos , I am not sure, I've been reading the docs:

In the example we can see how the first time we resolve Ninja, its property shuriken is undefined because no bindings have been declared for Shuriken and the property is annotated with the @optional() decorator.

After declaring a binding for Shuriken:

container.bind<Shuriken>("Shuriken").to(Shuriken);

All the resolved instances of Ninja will contain an instance of Shuriken.

So I think if the binding is declared, the @optional() decorator should make no difference in the behaviour, but if we remove the @optional() decorator, the behaviour changes.

If we look the current implementation of whenTargetIsDefault:

    public whenTargetIsDefault(): interfaces.BindingOnSyntax<T> {

        this._binding.constraint = (request: interfaces.Request) => {

            const targetIsDefault = (request.target !== null) &&
                (!request.target.isNamed()) &&
                (!request.target.isTagged());

            return targetIsDefault;
        };

        return new BindingOnSyntax<T>(this._binding);
    }

It seems to be all right but, looking at Target.isTagged() implementation:

    public isTagged(): boolean {
        return this.metadata.some((m) =>
            (m.key !== METADATA_KEY.INJECT_TAG) &&
            (m.key !== METADATA_KEY.MULTI_INJECT_TAG) &&
            (m.key !== METADATA_KEY.NAME_TAG) &&
            (m.key !== METADATA_KEY.UNMANAGED_TAG) &&
            (m.key !== METADATA_KEY.NAMED_TAG));
    }

It's kind of weird, if the target has the optional() decorator, is it supposed to be tagged? I think this is a bug and the right implementation would be:

    public isTagged(): boolean {
        return this.metadata.some((m) =>
            (m.key !== METADATA_KEY.INJECT_TAG) &&
            (m.key !== METADATA_KEY.MULTI_INJECT_TAG) &&
            (m.key !== METADATA_KEY.NAME_TAG) &&
            (m.key !== METADATA_KEY.UNMANAGED_TAG) &&
            (m.key !== METADATA_KEY.NAMED_TAG) &&
            (m.key !== METADATA_KEY.OPTIONAL_TAG));
    }

which would solve the issue, the optional() decorator would be transparent when there's a binding for the service and would give isTagged method more sense

[notaphplover]

Hi @rainerschuster the issue is fixed in the master branch now! :)

[rainerschuster]

Thanks for fixing!

[notaphplover]

You are very welcome :smiley: we expect to release a new version soon, I'm closing the issue. Feel free to ping me if anything is wrong and I'll reopen it immediately.