Kotlin / kotlinx.coroutines

Library support for Kotlin coroutines
Apache License 2.0
12.94k stars 1.84k forks source link

Should (at least some) basic operators be implemented specifically for StateFlow (e.g. map) #2081

Closed zach-klippenstein closed 4 years ago

zach-klippenstein commented 4 years ago

Calling stateFlow.map {…} will return a Flow, and lose the value property of the upstream StateFlow. Map is a very simple operator, and can be trivially implemented for StateFlow to also apply to the value property.

One use case that I've run into for map specifically is having a large AppState type that represents all/a large part of an app's "total" state, but wanting to expose a subset of that state to some specific code (e.g. a single screen).

Is it worth considering adding overloads of operators like map specific to StateFlow? This can also be solved with stateFlow.map(::foo).stateIn(scope, initialValue = foo(stateFlow.value)), but that's a little verbose.

Dominaezzz commented 4 years ago

Somewhat of a duplicate https://github.com/Kotlin/kotlinx.coroutines/issues/2008 .

Dominaezzz commented 4 years ago

It's actually more verbose than that I think. stateFlow.drop(1).map(::foo).stateIn(scope, initialValue = foo(stateFlow.value)). As you need to skip the emission of the current value when collecting, since you've specified the initial value already.

zach-klippenstein commented 4 years ago

Ah, Roman closed that issue with "Any chain of operators on a state flow can be materialized into another StateFlow using stateIn operator", so I guess that answers my question.

elizarov commented 4 years ago

It cannot be simplified further, as maintaining and computing a state requires a scope. You cannot just define map on StateFlow to return a StateFlow. You need to specify a scope in which the corresponding computation happens. And should do it once to materialize the whole chain of operations as opposed to launching a coroutine to compute each intermediate step of computation, which is inefficient.

zach-klippenstein commented 4 years ago

You don't need a scope to map a StateFlow though, the transform function can be independently applied to both the value property and the stream.

elizarov commented 4 years ago

@zach-klippenstein You cannot "independently" apply asynchronous transformations to the value and even if you limit them to only being non-suspending, then you'll have a hot of other problems like state.map { it + Random.nextInt(10) } will be showing inconsistent values with the stream. And if you throw things like filter into the mix, then you suddenly cannot implement it at all without precomputing in its own scope.

pacher commented 4 years ago

@elizarov I do understand your arguments against including it in the library.

But understanding the limitations, what would be the best way to implement my own StateFlow.map for very fast and simple, synchronous and non-suspending mapping which I am sure is idempotent?

Does this implementation looks good enough?:

inline fun <T, R> StateFlow<T>.mapState(crossinline transform: (value: T) -> R) = object : AbstractFlow<R>(), StateFlow<R> {
    override val value: R get() = transform(this@mapState.value)
    override suspend fun collectSafely(collector: FlowCollector<R>) {
        collector.emitAll(this@mapState.map { transform(it) })
    }
}

Are there any problems besides StateFlow being not stable for inheritance?

Using stateIn in not only verbose, it requires own scope which needs managing where all I want is simple mapping performed in the scope of the consumer like flow.map. The biggest advantage of StateFlow for me is that it is not only a flow but a generic container of value, meaning that it can be used outside of suspending world where I don't have a suitable scope at all.

elizarov commented 4 years ago

But understanding the limitations, what would be the best way to implement my own StateFlow.map for very fast and simple, synchronous and non-suspending mapping which I am sure is idempotent?

Why don't you just use a simple Flow in this case?

Are there any problems besides StateFlow being not stable for inheritance?

For now, that's a key problem. The StateFlow interface will change, becoming a descendant of SharedFlow (see #2034) and it will not implementable like this anymore.

Using stateIn in not only verbose, it requires own scope which needs managing where all I want is simple mapping performed in the scope of the consumer like flow.map. The biggest advantage of StateFlow for me is that it is not only a flow but a generic container of value, meaning that it can be used outside of suspending world where I don't have a suitable scope at all.

What's the actual use-case you have? What are you trying to do?

Dominaezzz commented 4 years ago

The StateFlow contract is going to get more complex if I remember correctly, but that is still an inheritance stability issue.

The other problem is distinctUntilChanged (and two others iirc) is a no-op for StateFlow (another one of those pesky contracts), so your mapState combined with distinctUntilChanged could break!

Also, state.mapState { it + Random.nextInt(10) } still won't work as expected when value is called.

Scope cannot be (nicely) avoided.

pacher commented 4 years ago

Why don't you just use a simple Flow in this case?

What's the actual use-case you have? What are you trying to do?

We have some rather complex state machines or maybe technically it is more of a state pattern. All kinds of external and internal events are represented as Flows. The internal complexity grows exponentially. With introduction of StateFlow I realized that the whole thing could be simplified dramatically. Instead of reacting to various events onXXX... individually, each state holds references to StateFlows of events it needs and have just simple update-like method where it then pulls the most recent values out of StateFlows to compute a new state. The Flow part of StateFlow is still used and collected at the level of the "machine" or "context", but it serves mostly as just a notification that something has changed to kick the computation cycle which will call that update method. Among other things this also allows to effectively conflate many unrelated updates. So, the collection and scopes and stuff is all there but on the higher level, where the outer "context" takes care of subscriptions etc. On the lower level there is just State which has StateFlow as a container from which it pulls the current value during update cycle. No scopes or coroutines there, trying to be as simple as possible.

Now, I have a SatetFlow<T?>, where data itself (events) is not nullable, but null represents the absence of initial value when no event happened yet. Later on some event did happen or I have a suitable value to use as default and I want to get rid of nullability before passing this StateFlow to the next state. So I came up with this:

private fun <T> StateFlow<T?>.withInitial(initial: T) = object : AbstractFlow<T>(), StateFlow<T> {
    override val value: T get() = this@withInitial.value ?: initial
    override suspend fun collectSafely(collector: FlowCollector<T>) {
        collector.emitAll(this@withInitial.map { it ?: initial })
    }
}

But since it is not exactly a "right" thing to do, I can not be completely sure in it. I don't know if I can test all the corner cases.

Also, state.mapState { it + Random.nextInt(10) } still won't work as expected when value is called.

Sure, because it is not idempotent. Since we can not describe idempotency with our type system, we can not have such an API exposed in the library. That is why I was asking for advice on how to implement this privately for myself, where I know that there are no randoms and can impose some clear limitations on usage :)