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 9 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.
BUT, the detection only works for the same module, if you separate the definitions into different modules, no exception thrown, and I can't figure out a way to detect it.

val mapKey1 = MapKey("key", 1)
val mapKey2 = MapKey("key", 2)

module {
    declareMapMultibinding<MapKey, Simple.ComponentInterface1> {
        intoMap(mapKey1) { Simple.Component1() }
    }
}

module {
    declareMapMultibinding<MapKey, Simple.ComponentInterface1> {
        intoMap(mapKey2) { Simple.Component1() } // NO exception thrown
    }
}

val map: Map<MapKey, Simple.ComponentInterface1> = koin.getMapMultibinding()
map[mapKey1] == null
map[mapKey2] != null

This is not the common use cases, maybe same module detection is enough?

@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 override
        intoMap("keyOfComponent1") { Simple.Component2() } // element override
    }
}

If we load modules with allowOverride = false, we must answer the question: Are multibinding overloads and element overloads considered overloads? If they are, then the current implementation is fine (but the DefinitionOverrideException messages 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 once and inject all elements within the same module.

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