koin icon indicating copy to clipboard operation
koin copied to clipboard

"includes" doesn't work when including modules which are defined later in the file

Open PaulWMR opened this issue 2 years ago • 2 comments

Describe the bug When declaring modules using the new 'includes' option, an IllegalArgumentException is thrown if the module that includes other modules is declared in the same file before the included module is declared. I would prefer to have my main module containing everything to be declared at the top of my file rather than at the bottom.

The exception is thrown in the function flatten(modules, newModules) in Module.kt. The message is "Flatten - No head element in list".

To Reproduce Steps to reproduce the behavior:

Create a Kotlin file with the following contents:

fun main() {
    startKoin {
        modules(mainModule)
    }
}

val mainModule = module {
    includes(module1, module2)
}
private val module1 = module {
//module definition here
}
private val module2 = module {
//module definition here
}

Run main to see the exception.

Expected behavior I expect Koin to start like it would if mainModule was declared after module1 and module2

Koin project used and used version (please complete the following information): koin-core version 3.2.0

Additional moduleDefinition

The following works fine without any issues:

fun main() {
    startKoin {
        modules(mainModule)
    }
}

private val module1 = module {
//module definition here
}
private val module2 = module {
//module definition here
}
val mainModule = module {
    includes(module1, module2)
}

PaulWMR avatar May 13 '22 09:05 PaulWMR

Yeah, it's a kotlin bug :/ We need to push it to the Kotlin team

arnaudgiuliani avatar Jun 27 '22 14:06 arnaudgiuliani

Same issue here.

kolyneh avatar Jun 28 '22 10:06 kolyneh

@arnaudgiuliani just for the sake of learning, May you explain how this is a kotlin bug ?

ahmed3elshaer avatar Nov 28 '22 14:11 ahmed3elshaer

@arnaudgiuliani I'm not an expert, but I assume that this is because Kotlin also initializes top-level variables in order, and if they are lower, then they will be initialized later

denchic45 avatar Dec 12 '22 11:12 denchic45

Hey @arnaudgiuliani, the problem is in fact a Kotlin "issue" but I do believe you can workaround it on Koin side.

First and foremost:

Now that we understand what is happening, we should be able to fix it by converting the module function to a lazy evaluation - that will ensure Kotlin will have the time to (static) initialize them all before the direct access to the instance (i.e., startKoin).

To exemplify my point, let's assume we introduce the following function:

fun lazyModule(builder: Module.() -> Unit): Lazy<Module> = lazy { module(builder) }

Now, we should be able to write the original example without causing any runtime issue:

val mainModule by lazyModule {
  includes(module1, module2)
}

private val module1 by module { TODO() }

private val module2 by module { TODO() }

That should fix the problem and can be a temporary workaround, but isn't the best solution from an API point of view: the module function will become redundant, and using the wrong combination (module, lazyModule and/or includes) may lead to runtime issues due to how Kotlin initialize top-level properties.

A better solution would be to rethink Module to be lazy itself, but that may require a major refactor of how Module works internally...

FYI: I did not run this code, and wrote it from my phone - keep in mind it may contain issues. If someone can confirm the code here works, we can start from there and iterate.

marcellogalhardo avatar Jan 17 '23 22:01 marcellogalhardo

Hi @marcellogalhardo ,

The fun stuff is that in Koin 2.x, modules were lazy declared. I was thinking the same as you. The interesting point is that I'm considering reworking this point toward loading scalability for such modules. It would be for Koin 4.0, as it's a major breaking. change.

arnaudgiuliani avatar Jan 19 '23 10:01 arnaudgiuliani

In my case, declaring the module this way works.

val module: Module get() = module { ... }

vjh0107 avatar Feb 11 '23 14:02 vjh0107

Confirming that the solution offered by @vjh0107 is working and in an elegant way. I have the habit to define the aggregate module at the top of the code for better readability as follows:

val appModules = module { mainModule, otherModule }
val mainModule = module {}
val otherModule = module {}

Here the use of mainModule ahead of its definition is not causing any issue. However, as I was migrating to using the new "includes" feature, I was getting the same error from this issue:

val appModules = module { includes(mainModule) }
val mainModule = module {}

Applying the workaround:

val appModules: Module get() = module { includes(mainModule) }
val mainModule = module {}

and all is again fine. Above example is heavily simplified from the original code which has lots of modules included.

Since this is a simple, probably commonly used module definition code pattern, I would guess many folks hitting this issue as they start using the (extremely useful --> big thank you) "includes" feature ...

ewaldc avatar Feb 27 '23 10:02 ewaldc

There is another solution that worked for me. If the included modules are in different files it doesn't break.

Breaks

val appModules = module { mainModule, otherModule }
val mainModule = module {}
val otherModule = module {}

works

AppModules.kt
val appModules = module { mainModule, otherModule }

MainModules.kt
val mainModule = module {}

OtherModule.kt
val otherModule = module {}

julio-souza avatar Aug 08 '23 19:08 julio-souza

Note has been added to the official doc + link to this issue to help with workaround

arnaudgiuliani avatar Sep 08 '23 14:09 arnaudgiuliani