-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Find replacement for DI #2475
Comments
IMHO I really like idea of dependency injection(DI) . fooReporter(/*config.client*/client) { return client } @dignifiedquire why do you want replace it? Only because people can not understand it? ✌️ |
@maksimr I am not sure, maybe just update things to use a newer library. If we are happy with DI itself I am happy to just move to a maintained library. |
After doing a short review of how often we actually use |
Main issue is, as you said, refactoring it out of Karma without breaking things for plugin authors. |
@dignifiedquire would you be open to a PR utilizing injection-js? Could also combine that feature with moving from pseudo-classes to proper ECMAScript classes, since that should make injection easier. Currently my TypeScript PR (#2893) is blocked by a bunch of ambiguous typing that is a result of pseudo-classing making things unclear. |
I'm curious about your definition of Line 11 in 39d378d
which defines methods on .prototype (should be converted to es6 classes) or like dotsLine 13 in 39d378d
Or both ;-) |
Both, although the former is more egregious. I was wrestling with type ambiguity in file-list the other night and the easiest solution was rewriting the whole thing as an ES6 class. |
|
I suggest tackling es6 and new DI separately, starting with a simple case, and developing a documented recipe for these changes. I guess the test coverage for the plugin space is not sufficient to ensure that a passing karma test will ensure plugins work. So patience and documentation will be essential. |
Does the old DI work with new class syntax? My concern is that rewriting them separately might raise unexpected compatibility issues and it might just be best to make any breaking changes simultaneously instead of in multiple steps. |
es6 classes can be used with es3 code; it may or may not be pretty and some investigation may be needed. Changing the DI is, I believe, a much bigger issue because it is part of how plugins interface with karma. We would need some kind of migration strategy where old and new DI are both allowed, then we reach-out/DIY the changes for the top plugins, then deprecate the old DI. |
The di (dependency injection) module we currently use is very outdated and the di setup we have is pretty hard to understand, especially for people new to the code base.
Currently all plugins though depend on it, so we need to find a backwards compatible solution or at least an interface that is as simple to use as the current one for plugin authors.
The text was updated successfully, but these errors were encountered: