fix(context): check constructor/method override for @inject #1596
Conversation
virkt25
left a comment
virkt25
left a comment
There was a problem hiding this comment.
Is there a way to set options.ownMetadataOnly? Should this behaviour be documented?
|
@virkt25 If you're asking whether users should have the option to look for the just the subclass' metadata, I think we should but maybe in a separate PR as a feature later on. |
|
Sort of, Just wanted to know if the user had any control over setting this flag or not. Also this behaviour should be documented somewhere in the docs so users know what to expect when extending a class using DI. |
| if (target.toString().match(/\s+constructor\s*\([^\)]*\)\s+\{/m)) { | ||
| options.ownMetadataOnly = true; | ||
| } | ||
| } else if (target.hasOwnProperty(method)) { |
There was a problem hiding this comment.
Is this code block covered in a test?
There was a problem hiding this comment.
According to Coveralls, this branch is executed several times (14x) by our test suite. However, I cannot tell if there is a test that would fail if this branch was not present.
@raymondfeng could you please add an explicit test for this branch?
|
@bajtos Any opinions? |
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
@raymondfeng the test case is legitimate. The implementation is a bit hacky, but I am ok with that since the ugliness is limited to a single place.
Fixes #1565
Checklist
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated