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

Implement new DSL for Factories instead of Providers only. #469

Open
Nek-12 opened this issue Jan 8, 2025 · 3 comments
Open

Implement new DSL for Factories instead of Providers only. #469

Nek-12 opened this issue Jan 8, 2025 · 3 comments

Comments

@Nek-12
Copy link

Nek-12 commented Jan 8, 2025

Similar to what is requested of Koin, we can implement a new and bindFactoryOf overloads that take not only the di instances into consideration, but also the parameters.

The way I envision it to work is:

  1. During new invocation, it creates a holder which will contain flag consumed that specifies whether the parameters have been used already.
  2. When the constructor is resolved, instead of using instance from DI directly, it first checks if a) the type of the argument matches the parameter type, and b) whether the parameters have not yet been consumed. If both are true, it provides parameters and marks them as consumed.
  3. The next invocation proceeds. If the argument with type matching parameter type is encountered again, the parameters are not injected a second time. Instead, we should try to find them in the DI container this time or fail immediately.

This results in the same concise syntax bindFactoryOf being available for use.

Would you like me to open a PR for this feature?

P.S. I apologize for issue spam, we're migrating to Kodein and this is a great opportunity to ensure feature parity with Koin.

@Nek-12
Copy link
Author

Nek-12 commented Jan 8, 2025

I have implemented a working solution in our repo.

@PublishedApi
internal interface ParameterizedNew {

    operator fun <T : Any> invoke(t: TypeToken<T>): T
}

@PublishedApi
internal inline fun <reified P : Any, T> DirectDIAware.parameterized(
    params: P,
    block: ParameterizedNew.() -> T,
) = object : ParameterizedNew {

    private val pType = generic<P>()
    private var consumed by atomic(false)

    @Suppress("UNCHECKED_CAST")
    override fun <T : Any> invoke(t: TypeToken<T>): T {
        if (consumed || !pType.isAssignableFrom(t)) return directDI.Instance(t, null)
        consumed = true
        return params as T
    }
}.run(block)

@PublishedApi
internal inline fun <reified T : Any> ParameterizedNew.invoke(): T = invoke(generic<T>())

public inline fun <T, reified P : Any, reified P1> DirectDIAware.new(
    param: P,
    constructor: (P1) -> T,
): T = parameterized(param) { constructor(invoke()) }

public inline fun <T, reified P : Any, reified P1, reified P2> DirectDIAware.new(
    param: P,
    constructor: (P1, P2) -> T,
): T = parameterized(param) { constructor(invoke(), invoke()) }

// 20 more overloads for new, +22 for factory and 22 for multiton.

I'm not sure how accurate it is, as I need to write tests for it when it is going to be moved to the main kodein repo. We will also need to settle on how to handle inheritance in the parameter injection. Using isAssignableFrom could be a more error-prone approach.

There is also an allocation overhead on each creation of a dependency because of the storage of the atomic consumed. This can be further optimized, but at the cost of less safety.

The resulting code looks like this:

    container<_, Params> { new(it, ::UserContainer) }

@romainbsl
Copy link
Member

Of course PR are welcome !
We can iterate together on the subject obviously, let me know if you manage some time, if not I'll try to start from what you've done.

Just curious, from what library and why are you migrating ?

@Nek-12
Copy link
Author

Nek-12 commented Jan 9, 2025

We're migrating from Koin. We're migrating because our team had enough with Koin.

Some reference:

Koin was #1 reason for crashes in our app and the sole cause of our low crash free session count.

I am glad that we don't have any crashes anymore with Kodein so far.

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

No branches or pull requests

2 participants