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

map & set multibinding #1951

Open
wants to merge 12 commits into
base: 4.1.0
Choose a base branch
from

Conversation

luozejiaqun
Copy link

@luozejiaqun luozejiaqun commented Aug 26, 2024

Implement map & set multibinding just like dagger2.
Here is some implementation details:

Every MapMultibinding & SetMultibinding will involve 3 definitions:

  1. the multibinding itself whose qualifier is mapMultibindingQualifier or setMultibindingQualifier
  2. the element in multibinding whose qualifier is multibindingElementQualifier (use mapMultibindingQualifier/setMultibindingQualifier as prefix)
  3. the MultibindingIterateKey which used to retrieve all elements, the key's qualifier is multibindingIterateKeyQualifier (also use mapMultibindingQualifier/setMultibindingQualifier as prefix)

When getting specific element from map multibinding, the map multibinding will try to retrieve the element through multibindingValueQualifier;
When getting all elements from map or set multibinding, the multibinding will first try to get all MultibindingIterateKeys and filter them based on multibindingQualifier, and then retrieve each element through multibindingElementQualifier.

The known problems:

  1. the multibinding qualifier can't be null, since it's qualifier will be used as prefix of element & iterate key
  2. the map multibinding doesn't support null as a key or value
  3. the elements order of multibiding is different with the order of element definitions Now, both set & map multibinding elements are iterated in the order they are defined.

And just like dagger2, multibinding is inherited (because MultibindingIterateKey can be retrieved from getAll), current scope's multibinding can get all elements that define in linked scope.

Here are some use cases:

module {
    intoMap<String, Simple.ComponentInterface1>("keyOfComponent1") {
        Simple.Component1()
    }
    intoMap<String, Simple.ComponentInterface1>("keyOfComponent2") {
        Simple.Component2()
    }

    intoSet<Simple.ComponentInterface1> {
        Simple.Component1()
    }
    intoSet<Simple.ComponentInterface1> {
        Simple.Component2()
    }
}

val map: Map<String, Simple.ComponentInterface1> = koin.getMapMultibinding()
val set: Set<Simple.ComponentInterface1> = koin.getSetMultibinding()

issue #772

@arnaudgiuliani arnaudgiuliani changed the base branch from 4.0.0 to 4.1.0 November 15, 2024 14:17
@arnaudgiuliani arnaudgiuliani added this to the 4.1 milestone Nov 15, 2024
@arnaudgiuliani
Copy link
Member

Looks interesting, will check for 4.1 👍

@luozejiaqun
Copy link
Author

luozejiaqun commented Dec 19, 2024

UPDATE:
the old implement is too error prone to write the following code:

module {
    intoMap("keyOfComponent1") {
        Simple.Component1() // implicitly declare Map<String, Simple.Component1>
    }
    intoMap("keyOfComponent2") {
        Simple.Component2() // implicitly declare Map<String, Simple.Component2>
    }

    intoSet {
        Simple.Component1() // implicitly declare Set<Simple.Component1>
    }
    intoSet {
        Simple.Component2() // implicitly declare Set<Simple.Component2>
    }
    /* Map<String, Simple.Component1> & Map<String, Simple.Component2>,
    Set<Simple.Component1> & Set<Simple.Component2> are different multibindings */
}

so I update the implement. Now elements can only be injected through explicit multibinding declaration and force type safety:

module {
    declareMapMultibinding<String, Simple.ComponentInterface1> {
        intoMap("keyOfComponent1") { Simple.Component1() }
        intoMap("keyOfComponent2") { Simple.Component2() }
        intoMap("keyOfComponent3") { Other() } // build error
    }

    declareSetMultibinding<Simple.ComponentInterface1> {
        intoSet { Simple.Component1() }
        intoSet { Simple.Component2() }
        intoSet { Other() } // build error
    }
}

The later element definition overrides the previous one, and the elements are iterated in the order they are defined.
@luozejiaqun
Copy link
Author

luozejiaqun commented Dec 30, 2024

The rare use cases:

data class MapKey(
    val name: String,
    val value: Int,
) {
    override fun toString(): String = name
}

module {
    declareMapMultibinding<MapKey, Simple.ComponentInterface1> {
        intoMap(MapKey("key", 1)) { Simple.Component1() }
        intoMap(MapKey("key", 2)) { Simple.Component1() } // MapMultibindingKeyTypeException will be thrown
    }
}

since multibinding implement uses key.toString() as a part of element & IterateKey qualifier, if two keys have different values but the same toString(), it defines one element actually. This is so weird and error prone, so an exception will be thrown.

module {
    declareMapMultibinding<MapKey, Simple.ComponentInterface1> {
        intoMap(MapKey("key", 1)) { Simple.Component1() }
    }
}

// declare in different modules
module {
    declareMapMultibinding<MapKey, Simple.ComponentInterface1> {
        intoMap(MapKey("key", 2)) { Simple.Component1() } // MapMultibindingKeyTypeException will be thrown
    }
}

@luozejiaqun
Copy link
Author

luozejiaqun commented Dec 31, 2024

The multibinding overrides issue:

module {
    declareMapMultibinding<String, Simple.ComponentInterface1> {
        intoMap("keyOfComponent1") { Simple.Component1() }
    }
}

module {
    declareMapMultibinding<String, Simple.ComponentInterface1> { // multibinding overrides
        intoMap("keyOfComponent2") { Simple.Component2() }
    }
}

Generally, you can declare elements across modules, but here, multibinding overloads actually occur. However, these multiple multibinding declarations are essentially the same.
But what if we load modules with allowOverride = false, here is the question: Are multibinding overloads considered overloads? If they are, then the current implementation is fine (but the messages of DefinitionOverrideException maybe a little unreadable). If not, then we need to exclude these cases during loadModules. But how to detect and exclude them? I lean towards the former, that is, in this scenario, you must declare multibinding and inject all elements within the same module.

@luozejiaqun
Copy link
Author

An explanation of elements order with example:

val module2 = module {
    scope(scopeKey) {
        declareMapMultibinding<Int, Simple.ComponentInterface1> {
            intoMap(2) { Simple.Component1() }
        }
    }
}
val module1 = module {
    scope(scopeKey) {
        declareMapMultibinding<Int, Simple.ComponentInterface1> {
            intoMap(1) { Simple.Component1() }
        }
    }
}

val module4 = module {
    declareMapMultibinding<Int, Simple.ComponentInterface1> {
        intoMap(4) { Simple.Component1() }
    }
}
val module3 = module {
    declareMapMultibinding<Int, Simple.ComponentInterface1> {
        intoMap(3) { Simple.Component1() }
    }
}

startKoin {
    modules(module1, module2, module3, module4)
}

val map: Map<Int, Simple.ComponentInterface1> = scope.getMapMultibinding()
// in elements definition order, but linked scope (root scope) elements first then current scope elements
// module definition order is irrelevant, but module loading order is key.
map.keys.toList() == listOf(3, 4, 1, 2)

It's not that I intentionally maintain the order, but this order is crucial for element overloading. For example:

val component = Simple.Component1()
val module44 = module {
    declareMapMultibinding<Int, Simple.ComponentInterface1> {
        intoMap(4) { component }
    }
}

modules(module1, module2, module3, module4, module44)

map[4] === component // later element definition wins

@arnaudgiuliani
Copy link
Member

arnaudgiuliani commented Jan 9, 2025

@luozejiaqun it's a great proposal, I would like to review naming/API to propose something more compact.

I opened KFIP https://github.com/InsertKoinIO/KFIP/ to open design process to public. This could be a good first proposal that we can design together. I can start a draft and we can work together on this Map/Set injection feature.

We can talk also on #koin-contributors channel

@luozejiaqun
Copy link
Author

@arnaudgiuliani I'm looking forward to talking more about this with you, and I do have a few questions. I tried making a PR on KFIP, but writing so much English technical documentation is a little difficult for me.

@arnaudgiuliani
Copy link
Member

Let me bootstrap it, this should frame our discussion around 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants