InversifyJS icon indicating copy to clipboard operation
InversifyJS copied to clipboard

Looks like incorrect behavior is in _validateActiveBindingCount

Open tryvols opened this issue 5 years ago • 2 comments

https://github.com/inversify/InversifyJS/blob/65596eaa2915787d14f852992539a957acc23af0/src/planning/planner.ts#L116

      switch (bindings.length) {
         // ...
         case BindingCount.OnlyOneBindingAvailable:
            if (!target.isArray()) {
                return bindings;
            }
        case BindingCount.MultipleBindingsAvailable:
        default:
            if (!target.isArray()) {
                // ...
            } else {
                return bindings;
            }
      }

If bindings.length === BindingCount.OnlyOneBindingAvailable and:

  • target isn't array - function returns bindings
  • target is array - we fall through to default condition, then in else block function returns bindings

So, in case BindingCount.OnlyOneBindingAvailable - function always returns bindings.

Looks like it's not correct behavior.

tryvols avatar Dec 13 '20 18:12 tryvols

Hi @tryvols I'll have a look at it :)

notaphplover avatar Apr 19 '21 12:04 notaphplover

Hi @tryvols after having a look we could simplify the code, but the behavior is correct

What means that target is Array

If look at Target implementation:

public isArray(): boolean {
    return this.hasTag(METADATA_KEY.MULTI_INJECT_TAG);
}

It means this service is being requested in a multi inject context. In this case we should return the array of bindings even if this is a multiInject context and we only got one binding. In the following code:

import { Container, inject, injectable } from 'inversify';

@injectable()
Class A {}

@injectable()
Class B {
  @multiInject('a')
  constructor(
      private readonly a: A[]
  )
}

const container = new Container();
container.bind('a').to(A);
container.bind('b').to(B);

const b = container.get<B>('b');

We expect b to be injected with an array of one A element even if it's a multi inject context

For the same reason container.getAll('a') should return an array of one A element, even if we are in a @multiInject context.

We have plans to refactor this

notaphplover avatar Apr 19 '21 12:04 notaphplover