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

Looks like incorrect behavior is in _validateActiveBindingCount #1271

Open
tryvols opened this issue Dec 13, 2020 · 2 comments
Open

Looks like incorrect behavior is in _validateActiveBindingCount #1271

tryvols opened this issue Dec 13, 2020 · 2 comments

Comments

@tryvols
Copy link

tryvols commented Dec 13, 2020

case BindingCount.OnlyOneBindingAvailable:

      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 tryvols changed the title Looks like incorrect check is in _validateActiveBindingCount Looks like incorrect behavior is in _validateActiveBindingCount Dec 14, 2020
@notaphplover
Copy link
Member

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

@notaphplover
Copy link
Member

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

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

No branches or pull requests

2 participants