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

Added new feature to disable base class checking #841

Merged
merged 5 commits into from
Apr 13, 2018

Conversation

jacobEAdamson
Copy link
Contributor

Description

Added an option to the container to skip checking base classes for @Injectable classes.

Related Issue

These issues are all related to this particular problem, but are most likely using workarounds:

#528
#297
#729

This issue is still open, and quite accurately explains why this feature is desirable:

#522

Motivation and Context

This change is a simple quality of life fix for those who are creating injectable
classes by extending classes provided by third parties. The only reason this check is
currently in place is to assist developers who may forget to call super() with the correct
arguments to instantiate the parent class. For those developers who wish to forgo those built
in checks, this options should save them the trouble of decorating all parent
classes with @injectable/@Unmanaged (since this quite tedious, and may cause unforseen problems
with 3rd party libraries).

How Has This Been Tested?

I have added a simple test in the planner test file, and have run the rest of the test suite.
It behaves as expected without the option (throws an error on base classes that are not @Injectable), and behaves as expected with the option.

Types of changes

  • Updated docs / Refactor code / Added a tests case (non-breaking change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@remojansen
Copy link
Member

Hi @jacobEAdamson awesome PR! thanks a lot for this contribution 💯

@remojansen remojansen merged commit 83eb36b into inversify:master Apr 13, 2018
@jacobEAdamson
Copy link
Contributor Author

jacobEAdamson commented Apr 13, 2018 via email

@rtang03
Copy link

rtang03 commented Apr 16, 2018

How can I install this latest build? The npmjs repo does not include this feature yet.

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

Successfully merging this pull request may close these issues.

4 participants