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

feat(dependency injection): Add dependency injection for plugins #1313

Merged
merged 40 commits into from
Jan 23, 2019

Conversation

nicojs
Copy link
Member

@nicojs nicojs commented Jan 7, 2019

This PR adds dependency injection to Stryker, in preparation of removing the side effects from the way we now load plugins. Instead of registering yourself to the correct factory (for example, by calling ReporterFactory.instance().register), it relies on plugins exporting a property called strykerPlugins.

Just to be clear: this PR does fix not the issue reported in #667. It only adds the dependency injection mechanism to Stryker. We still need to do PR's for each package (can go relatively fast)

The content of this PR:

  1. Add a dependency injection framework called typed-inject to the packages folder; A type safe dependency injection framework
  2. Add a package stryker-api/di which contains dependency injection related interfaces to help plugins know what to (type-safely) inject
  3. Add typed-inject as a dependency to the core Stryker package and use it. Reporter plugins are loaded via this new mechanism.
  4. Alter the PluginLoader in order to enable dependency injection in all plugins. It is still backward compatible with the old way of loading plugins.
  5. Add a package to contain generic test helpers called @stryker-mutator/test-helpers. This package contains a testInjector to help inject common stuff into injectables for testability purposes.
  6. It updated the BroadcastReporter and all build-in reporters to now use the new dependency injection mechanism (as a demo implementation and tryout mostely).
  7. It changed the HtmlReporter to work with the new way of loading plugins

There is still some work to be done. However, this PR can already be reviewed for the most part. Not changing a lot anymore.

  • Split of the HtmlReporter implementation so it can be reviewed in separate PR. -> feat(html-reporter): Remove side effects from html reporter #1314
  • Document typed-inject in its own readme
  • Add integration tests to typed-inject to make sure un-typesafe dependency injection results in compiler errors
  • Remove specific helper methods per plugin: reporterPlugin, configEditorPlugin, ... in favor of one plugins helper method. I think this is possible Unfortunately not possible. Moved the methods to "plugins" package instead

* Created `Injector` class, a readonly class that can instantiate `Injectable`s
* Updated the `PluginLoader`, so it can load plugins with the new way of working as well as provide a Bridge plugin for all "old fashion" plugin
* Started implenenting the `BroadcastReporter` and other Reporters in the new way.

Couldn't help to make some quality of life improvements:

* `StrykerOptions` now have the expected required/not required properties.
* Created "test-helpers" package so we can share test helpers between packages.
The TestInjector is a new way of creating your System Under Test (sut) objects.
You can configure what to inject and then call `TestInjector.inject(SutClass)`
Also added some unit tests for the Injector class.
* Made the context of the Injector generic.
* Made injector readonly and type safe
* Moved all injector related stuff to its own package: 'typed-inject'
* Updated TestInjector class to now use the new typed-inject
* Updated Stryker to now use the new typed-inject
* Updated Stryker-api to now use types from typed-inject
@ghost ghost assigned nicojs Jan 7, 2019
@ghost ghost added the 🔎 Needs review label Jan 7, 2019
Copy link
Member

@simondel simondel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed it 🎉
Could you take a look at my comments?

packages/stryker-api/src/plugin/Contexts.ts Outdated Show resolved Hide resolved
packages/stryker-api/src/plugin/Plugins.ts Outdated Show resolved Hide resolved
packages/stryker-util/src/tokens.ts Outdated Show resolved Hide resolved
packages/stryker/src/Stryker.ts Outdated Show resolved Hide resolved
packages/typed-inject/README.md Outdated Show resolved Hide resolved
packages/typed-inject/README.md Outdated Show resolved Hide resolved
packages/typed-inject/README.md Outdated Show resolved Hide resolved
packages/typed-inject/README.md Outdated Show resolved Hide resolved
packages/typed-inject/README.md Show resolved Hide resolved
@nicojs
Copy link
Member Author

nicojs commented Jan 22, 2019

I've implemented all review comments I think.

Thanks 👍

@simondel
Copy link
Member

Awesome :) Thanks. Feel free to merge 👍

@nicojs nicojs merged commit f90cd56 into master Jan 23, 2019
@ghost ghost removed the 🔎 Needs review label Jan 23, 2019
@nicojs nicojs deleted the 667-remove-side-effects branch January 23, 2019 08:28
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.

3 participants