stephanenicolas / toothpick

A scope tree based Dependency Injection (DI) library for Java / Kotlin / Android.
Apache License 2.0
1.12k stars 115 forks source link

Using toothpick in libraries #286

Closed lukaville closed 6 years ago

lukaville commented 6 years ago

I'm trying to use toothpick in a library project (no toothpick classes/objects exposed to the clients of the library). But I faced a problem that if a client of the library uses preventMultipleRootScopes configuration it will fail because the library opens new root scope. Also, we can't set isolated toothpick configuration in the library and in the client project because ConfigurationHolder.configuration field is static. How can I use multiple toothpicks with different configuration at the same time? Is it intended to use tootpick in this way?

stephanenicolas commented 6 years ago

TP is not intended to be running with multiple configuration, this doesn't make sense at all. TP uses static fields to ensure there is a single configuration.

It's not normal on Android to open multiple root scopes. The option is there to be more permissive in java, but for android the scope tree is unique and start with the app scope as a root. There is no use case for multiple root scopes on android and multiple root scopes are an indication of something going bad when opening scopes.

If you are ok with this answer, please close the issue. If you think there is a legitimate use case for opening multiple root scopes, let us know but it would be really really really surprising.

stephanenicolas commented 6 years ago

To be a little more precise on this:

when you create a scope you do:

TP.openScopes(app, activity);
//install modules, etc.

when you use a scope, not creating it but just using it, you do:

TP.openScope(activity);
//use the scope

openscope is a nice method has it returns a scope, creating it if needed. But it means that you can actually create scopes when you want to use scopes. A very common use case it to use openscope in a background thread, and this gets executed right after you activity was destroyed.

In this case, openscope will not return the activity scope that was created under the app scope (because the activity got destroyed and its scope got closed), but it will return a new root scope starting at the activity level.

To detect such issues, we have the configuration to detect multiple root scopes. This will help you to detect where your code is using TP wrong.

TP fully supports concurrent injection, but we have to take care on Android of not using an object after it died in its lifecycle.

lukaville commented 6 years ago

Let's say I have a completely separated java library and I want to use toothpick to manage its internal dependencies. In the library I call Toothpick.setConfiguration and open scope using Toothpick.openScopes(LIBRARY_SCOPE). Then I want to use this library in an Android app. My app also calls Toothpick.setConfiguration with own configuration and opens own root Application scope.

As things stand now, we can't use this library if the client of the library (Android app) also use toothpick. It will work fine if Android app will not use toothpick. I can also face the same problem if I decide to use toothpick internally in the second library and use this two libraries simultaneously.

I can partially resolve the problem by disabling multiple root scopes check, but this check is really helpful and I don't want to accidendtely miss opening new root scope in the Android app.

stephanenicolas commented 6 years ago

@lukaville , your libs should not play with the configuration of TP. Only one point in the app should do that, and it should be done in App's onCreate method.

Regarding Toothpick.openScopes(LIBRARY_SCOPE), you are creating a root scope by doing this. I am wondering why you need this in your lib. If you lib components inject something, you could consider using the app scope, or passing them the scope they should use, or using a standard scope. For instance you can provide a component in your lib that is a @Singleton and then make it inject stuff. This will be injected using the app scope, and your app can provide the necessary bindings into the app scope when it creates it (in app's onCreate method as well).

I would really discourage you to open a scope in a lib. If you really really really want to do so, it means your app should create its appscope under the lib's scope. But this creates quite a semantic coupling indeed.

lukaville commented 6 years ago

I want to use this library in two apps: one app use toothpick and the second one doesn't (it uses dagger2). In my library, I have a few internal components that I want to inject into each other and toothpick does a great job of dealing with it. I don't want to force the clients of my library to use toothpick. I think there should be a possibility to use multiple isolated toothpicks with different configurations and with own root scopes.

stephanenicolas commented 6 years ago

I would not use dependency injection in a standalone library, even less a OSS library. I don't know any lib doing that, and probably for good reasons... ;)

pshmakov commented 6 years ago

@stephanenicolas Wait, but how is a library any different from an application? DI is there to help deal with complexity, so if the library is complex, it can benefit from DI. It's not uncommon to extract a library with some common behavior or even a part of UI and reuse it across multiple applications. I know examples where these extracted libraries are very complex and using DI there makes a big difference. Using TP in such a library should be an implementation detail, hidden from the clients of the library. E.g. the client should not have to somehow pass root scope name into the library in order to pass the multiple scope roots check (in case the client uses TP as well), let alone do any work on satisfying some dependencies used inside the library.

So I think this is a completely valid feature request.

I can imagine a solution could be to add Toothpick.createScopeTree() method and move the scope management including openScope method and multiple root checking from statics into ScopeTree objects. The scope trees would be completely independent.

stephanenicolas commented 6 years ago

A library is only different because you want developers to use it in any app. And good libs to put too much constraint on your code, like inheritance, or forcing you to use DI.

It's pretty simple for a lib that doesn't use DI to wrap it in DI, write providers to get the new instances of the classes of the lib. That's basically it. I would probably never pick a lib that forces me to use a given DI framework or another. I would simply discard it.

If it's completely hidden inside the lib, why not, but I don't think it's a very great design for a lib. Usually you try to avoid this and have simple libs, with few dependencies. It also avoids conflicts when people use the same lib but with a different version.

2018-03-14 9:32 GMT-07:00 pshmakov notifications@github.com:

@stephanenicolas https://github.com/stephanenicolas Wait, but how is a library any different from an application? DI is there to help deal with complexity, so if the library is complex, it can benefit from DI. It's not uncommon to extract a library with some common behavior or even a part of UI and reuse it across multiple applications. I know examples where these extracted libraries are very complex and using DI there makes a big difference. Using TP in such a library should be an implementation detail, hidden from the clients of the library. E.g. the client should not have to somehow pass root scope name into the library in order to pass the multiple scope roots check (in case the client uses TP as well), let alone do any work on satisfying some dependencies used inside the library.

So I think this is a completely valid feature request.

I can imagine a solution could be to add Toothpick.createScopeTree() method and move the scope management including openScope method and multiple root checking from statics into ScopeTree objects. The scope trees would be completely independent.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stephanenicolas/toothpick/issues/286#issuecomment-373087469, or mute the thread https://github.com/notifications/unsubscribe-auth/ABv33bf4V9vStyhKINeF03cEIBll5r03ks5teUYogaJpZM4SaDCo .

pshmakov commented 6 years ago

@stephanenicolas

Usually you try to avoid this and have simple libs, with few dependencies.

But what if you can't? Here's an example: https://play.google.com/store/apps/details?id=com.yandex.browser https://play.google.com/store/apps/details?id=com.yandex.zen https://play.google.com/store/apps/details?id=com.yandex.launcher In all 3 apps you can find a kind of news feed. It's more or less the same thing backed by the same library. And the library is very complicated, believe me :) It's essential to have in there a good level of modularity and testability, which is hard to achieve without DI. There is no option for the client apps to not use the library, and enforcing the same DI framework on all client apps is not an option either. This situation is not exceptional in my experience, big companies tend to put effort in making features isolated and reusable across several apps they develop. What would be your approach to such situation?

arkivanov commented 5 years ago

Any one knows why this completely valid feature request was closed? At my company we are now struggling because of this. We have a number of complex libraries that should be shared across the company and they are using TP. TP is not exposed outside of the libraries. Some projects use TP and there are no problems for them as they are just passing root scope to these libraries. But some projects use Dagger 2, and they also have to depend on TP so they can just open empty root scope. It's a very very big con of the TP so we are now considering migrating our TP projects to Dagger 2.

lukaville commented 5 years ago

@arkivanov yes, let's just use dagger :)

arkivanov commented 5 years ago

Looks like TP 2.0 is still static 😑

stephanenicolas commented 5 years ago

Yeap, we didn't work on this at all in TP2. Our effort was mostly focused on build times and incremental annotation processing.

Personnally, I don't wanna be a blocker for features that I don't really want to see in TP. TP also belongs to his community and for sure as main contributors we don't see all the requirements and possible usages of "our" lib. So, feel free to submit a PR if you need such a feature.

One thing I was wondering when reading this thread is, why don't you just shade TP jars in your lib ? Make it so that all TP packages are renamed at build time. You would then get a private TP with it's private scope that wouldn't interfere with what the libs users would be doing.