InsertKoinIO / koin

Koin - a pragmatic lightweight dependency injection framework for Kotlin & Kotlin Multiplatform
https://insert-koin.io
Apache License 2.0
9.08k stars 719 forks source link

Context.getKoin() is not always convenient. #12

Closed Jeevuz closed 7 years ago

Jeevuz commented 7 years ago

Hi! I have the case when I need to get dependency from the Koin and provide it to the factory method (abstract in super class). And at the moment this method called there is no context in Conductor's Controller yet. So I was forced to call StandAloneContext.koinContext.get().

Maybe it will be better to have getKoin() from everywhere if it is just an object (StandAloneContext)?

arnaudgiuliani commented 7 years ago

The main reason to not have KoinContext from everywhere is semantic: koinContext is used to help make injection with "by inject()" in Android classes. If you need dependency elsewhere, you should do it by constructor injection.

I don't understand well, how do you plan to make injection here. You should have to use the stand alone context directly. Can you provide a snippet?

Jeevuz commented 7 years ago

I have a library that help to make presenters outlive the rotation of the device. So it is responsible for the presenter lifecycle. And it need to create the presenter as soon as possible. It happens in onCreate of the Controller (the Conductor library "fragment"). Controller working with this lib need to implement method providePresenter returning newly created presenter.

The problem is that Activity/Fragment/Controller doesn't know when this method will be called. It just provides (creates) the presenter. And for the Controllers the getActivity method will return null at this time (before view attached to window).

And within providePresenter I need to set the Koin property used in the presenter's constructor in provide {}. So I think I can't use inject methods.

Hope I get it clearer. Sorry, I have no notebook around right now and can't send the code snippet. I'll send it later.

arnaudgiuliani commented 7 years ago

Ok, I thin I begin to see. But I still need a concrete snippet to help you. Paste it when you have the time.

Jeevuz commented 7 years ago

Here it is. We use RxPM (reactive presentation model pattern) and Conductor. So Screen is Conductor's Controller and PresentationModel is like a Presenter.

class AnythingScreen(args: Bundle) : Screen<AnythingPm>(args) {

    companion object {
        private val ARGS_NUMBER = "arg_number"

        fun newInstance(number: Int) = AnythingScreen(
                Bundle().apply {
                    putInt(ARGS_NUMBER, number)
                }
        )
    }

    override val screenLayout = R.layout.screen_anything

    override fun providePresentationModel() =
        StandAloneContext.koinContext
            .apply { setProperty(PmModule.ANYTHING_NUMBER, args.getInt(ARGS_NUMBER)) }
            .get<AnythingPm>()

    override fun onBindPresentationModel(view: View, pm: AnythingPm) {
        // binding here...
    }
}

The model:

class PmModule : AndroidModule() {

    companion object {
        const val ANYTHING_NUMBER = "anything_number"
    }

    override fun context() = applicationContext {
        provideFactory { AnythingPm(getProperty(ANYTHING_NUMBER), get(), get()) }
    }
}

Inside the used library we have such code for base Controller:

    ...

    final override val presentationModel get() = delegate.presentationModel

    final override fun onCreateView(inflater: LayoutInflater, container: ViewGroup, savedViewState: Bundle?): View {
        val view = createView(inflater, container, savedViewState)
        delegate.onCreateView() // This will cause call to providePresentationModel() method.
        return view
    }

    ...

While posting this I found that onCreateView() is called after we have the activity (after onContextAvailable() method), so in fact I can use the getActivity().getKoin(), but I still think it's not really good because in this way I rely on my knowledge of from where this method is called. In my opinion not to rely on this is better.

Anyway this is the case when somebody can not be able to use by inject<>().

What you think about it? Maybe there is another way I just cannot see?

arnaudgiuliani commented 7 years ago

Ok. effectively, you shouldn't have to access the stand alone context directly. koin-android provide inject/property lazy injectors for Activity, Fragment & Service (Android classes). You can access koinContext from Android context objects.

I try to limite things on Android side, and allow to pass the koinContext with context.getKoin() - else you should be able to make DI by constructor.

As I can see, you are using a framework to help you build your controller layer. First option to check: does your "AnythingScreen" class is accessible from Android, to give the Koin context from a Android context (avoid to use the standalone context from anywhere, you will reduce your component testability and depend too much on injection framework)?

Other option, you make your own extension for your framework (and avoid directly hit the stand alone context). You could make an extension on Screen like:

inline fun <reified T> Screen.inject(name: String = "") = lazy { StandAloneContext.koinContext.get<T>(name) }

This way, you allow injection only on those kinds of components.

Jeevuz commented 7 years ago

Personally I don't really like to use inject here because the property holding it is not really needed, because it used one time only:

...
    private val pmToProvide by inject<AnythingPm>()

    override fun providePresentationModel() = pmToProvide // used only here
...

So for me it is easier to call context.getKoin().get<>(). But in case with nullable activity property in the Conductor's Controller this will look not so good: activity!!.getKoin().get<>(). And also I think there can be the case when activity can be null. That why I started this thread.

Plus I need to set the property before I will get the dependency. And this another reason I don't want to use inject<>() here. Anyway I need to get konContext to set property, and need a context for that.

But I really understand your intent for having only context.getKoin(). And maybe it will be even better to make it more closed by use of internal modifier for the StandAloneContext. This way there will no such "me" trying to use it :)

As the result of this conversation, I will use the activity!!.getKoin().get<>() variant for my case.

PS. Also I think it will be good to close internals of the library with internal or private modifiers where possible. Because now is possible to access almost everything (eg. I can get StandAloneContext.koinContext.beanRegistry.definitions), and it seems not really good for the library.

arnaudgiuliani commented 7 years ago

I keep in mind your last point. Thanks.