Allow "on changed" Flow emission for more general MutableMap

[twyatt]

• Previously wrapped ConcurrentHashMap
  • Changed to allow more general usage by wrapping MutableMap instead
• Binary incompatible change due to type of constructor default value changed
  • Marked constructor internal to force usage of extension function and simplify similar changes in the future

[codecov]

Codecov Report

Merging #13 into master will increase coverage by 3.57%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##             master      #13      +/-   ##
============================================
+ Coverage     67.85%   71.42%   +3.57%     
+ Complexity       18       17       -1     
============================================
  Files             2        2              
  Lines            28       28              
  Branches          4        4              
============================================
+ Hits             19       20       +1     
+ Misses            9        8       -1     

Impacted Files	Coverage Δ	Complexity Δ
collections/src/main/kotlin/FlowMutableMap.kt	86.95% <87.50%> (ø)	17.00 <1.00> (?)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e9b8ff...0871d4f. Read the comment docs.

[Chris-Corea]

I might be forgetting something, but what if the onChanged emitted a non-mutable copy?

If a listener wanted a mutable copy of the map, they could just make a local mutable copy, right?

I guess my question is: why emit a mutable copy of the map?

[twyatt]

It's actually emitting a Map (not MutableMap):

tuulbox/collections/src/main/kotlin/FlowMutableMap.kt

Line 18 in 0f2e7e8

val onChanged: Flow<Map<K, V>> = _onChanged.filterNotNull()

MutableMap is just a facade though, so unfortunately every Map is also mutable if casted to MutableMap (thanks Java). So enforcement is more at the level of just making it more difficult to modify.

...but, by copying the original MutableMap ensures that any changes done to the emitted map (from the onChanged event) via casting means developer will be modifying the copy, not the original. Thereby protecting the expected dataset in the original map.

[Chris-Corea]

Ah thanks for the explanation. Flow<Map<K, V>> 🤦‍♂️

[cedrickcooke]

@twyatt if we're willing to JVM-lock, there is always Collections.unmodifiableMap(yourMap) to return a runtime-enforced read-only wrapper (not a copy, reads through) map. Because it's a Java function you can still cast it to MutableMap, but mutate attempts will at least fail at runtime.

[twyatt]

@cedrickcooke ohh, I do like that better; would presumably be more efficient than a copy.
I'll make that change.
I'm fine with the JVM-lock; as going multiplatform wouldn't be difficult and just entail a basic expect / actual function to provide the platform specific "immutable collection" for the change event.

[twyatt]

Oh wait, Collections.unmodifiableMap(yourMap) won't adhere to the desired functionality. It's just a "view" of the underlying Map.

I need the emitted Map to reflect the collection at a point in time.

I believe Collections.unmodifiableMap(yourMap) would produce different values over time if the underlying Map was changing; so doesn't properly represent an onChanged "event".

[cedrickcooke]

Since the same map is shared by all observers of the flow it might make sense to copy it and then wrap it in the unmodifiable view. I'm not sure if that's too defensive or if it's still reasonable.

[twyatt]

Since the same map is shared by all observers of the flow it might make sense to copy it and then wrap it in the unmodifiable view.

Ohhh ya. Very good point.

I'm not sure if that's too defensive or if it's still reasonable.

I think it's definitely reasonable; I'd rather start too defensive and later lift restriction if needed (e.g. performance reasons or something). Almost always easier to lift restrictions than to impose them later.