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

Support @Optional parameters #411

Closed
shlomiassaf opened this issue Nov 14, 2016 · 4 comments
Closed

Support @Optional parameters #411

shlomiassaf opened this issue Nov 14, 2016 · 4 comments
Assignees
Milestone

Comments

@shlomiassaf
Copy link

Same as @Optional in Angular 2.

Regarding the Avoid optional dependencies in Good practices

In other words, there is a constructor that accepts dependency injection, but also another constructor that uses a "default" implementation. This also violates the DIP and tends to lead to LSP violations as well, as developers, over time, start making assumptions around the default implementation, and/or start new-ing up instances using the default constructor.

I couldn't understand if its referring to this idea... if it does, I would like to note that inversify should be a library for developers building their applications but also for developers building libraries with the help of inversify.

When building a library, injections can be optional, this is a common use case.

I believe @Optional should be included, even if in some cases its not recommended.

@remojansen
Copy link
Member

Thanks for this suggestion. I see your point but I really think that optional dependencies lead to bad code that's why is not available at the moment. Have you encountered a use case in which you need @optional ?

If there is a real use case in which it is needed I will be happy to implement it but I would like to avoid it otherwise.

As a side note: When I started to think about @optional I also though about @default("ID", value) which will resolve and inject "ID" but if no bindings are declared will inject value.

@shlomiassaf
Copy link
Author

@remojansen Bad as it might be it is something that in several scenarios is needed. Again, as a library author I would like to inject some values but not always, depending on state and configuration in a dynamic way.

For example, a meta wrapper for an express server can be configured to listen to 2 http servers: secured (https) and unsecured (http), both derive from net.Server but not from each other.
A user might use http, https or http + https... so I want to provide him the option to set them both in the ctor...
I can create a token (service identifier) for an array of net.Server but then he needs to find the type, no good...

My current solution is to bind both types and set one of the to undefined...

About @default, I think it can be

class MyClass {
  constructor(@inject("ID") @default("value") myInjection: string) {
  }
}

It's better to keep a consistent API for more verbosity.

The @default parameter can be optional giving you and @optional behaviour

class MyClass {
  constructor(@inject("ID") @default() myInjection: string) {
  }
}

Though I would still add an @optional decorator for code clarity, it is easier to understand.

My 2 cents on this is to be opinionated but not aggressively.
Just because people can mess up thing doesn't mean others should suffer.
Inversify is not a somewhat of a low level library. When used, it's part of the core of the application and as such should offer flexibility.

@remojansen remojansen added this to the 3.0.0 milestone Nov 29, 2016
@remojansen
Copy link
Member

Sorry for the late reply I wanted to finish other features first. I've been thinking about this and here is my idea of how the API should look like.

@default()

I don't think InversifyJS should manage default values because the TypeScript compiler can do this job for us and one of our design goals is to add as litle runtime overhead as possible. This means that we only need the @optional annotation and the TypeScript syntax for default values = "value":

class MyClass {
  constructor(@inject("ID") @optonal() myInjection: string = "value") {
  }
}

@optional()

We need this decorator and it will be implemented. I recommend using @optional() with non- nullable types enabled.

type Optional<T> = T | undefined;

The type of the arguments decorated @optional() should be the union of undefined and a give type:

class MyClass {
  constructor(@inject("ID") @optonal() myInjection: Optional<string>) {
  }
}

@remojansen remojansen self-assigned this Dec 3, 2016
@remojansen
Copy link
Member

Hi @shlomiassaf optional dependencies have now been merged into master by #432 and will be available in the 3.0.0-beta.4 release later this week 😉

remojansen added a commit that referenced this issue Dec 4, 2016
remojansen added a commit that referenced this issue Dec 5, 2016
* Implemented #411

* 3.0.0-rc.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants