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

toConstructor @injectable() should not required @inject annotations #338

Closed
remojansen opened this issue Aug 14, 2016 · 0 comments · Fixed by #339
Closed

toConstructor @injectable() should not required @inject annotations #338

remojansen opened this issue Aug 14, 2016 · 0 comments · Fixed by #339
Milestone

Comments

@remojansen
Copy link
Member

Expected Behavior

If missing @inject annotations we should throw a MISSING_INJECT_ANNOTATION error. But if the binding is a toConstructor then no @inject annotations should be required.

Current Behavior

If missing @inject annotations we always throw an error. Users can use @inject annotation to fix the issue but these annotations are not required.

Possible Solution

Do not throw error is toConsturctor binding.

Steps to Reproduce (for bugs)

let kernel = new Kernel();
kernel.bind<Newable<any>>("Newable<Category>").toConstructor(Category);

@injectable()
export class Category {
constructor(
    id: string,
    title: string,
    categoryFirstPermalink: string,
    categoryPermalink: string,
    pagination: number,
    categorySortingFn: ICategorySortingFn,
    contentSortingFn: IContentSortingFn,
    belongsToCollection: Collection
  ) {}
}

let expectedThisToBeTheConstructor = kernel.get(Category);
// Error: Missing required @inject or @multiInject annotation in: argument 5 in class Category.

Context

This issue was reported by @ubenzer

@remojansen remojansen added this to the 2.0.0 milestone Aug 14, 2016
@remojansen remojansen mentioned this issue Aug 14, 2016
11 tasks
remojansen added a commit that referenced this issue Aug 14, 2016
* Fixed #338

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

Successfully merging a pull request may close this issue.

1 participant