add collectWhileInState functions with flow builders #182

gabrielittner · 2021-08-03T20:27:13Z

Closes #161

This adds an collectWhileInState overload that flowBuilder: (Flow<InputState>) -> Flow<T> as parameter instead of a simple Flow<T>. The implementation is based on the same operator that onAction uses. This allows us to cancel it when the state changes without missing any actions while we are in the action. Any action emission is mapped into the current state and distinctUntilChanged is used to filter out emissions where the state did not change. The resulting Flow is then passed to flowBuilder and the return value of that is what we use like usually.

While I was adding the overload to InStateBuilderBlock I've also updated the docs on all methods, added overloads without flatMapPolicy (closes #179) and renamed block to handler.

gabrielittner · 2021-08-03T20:39:15Z

The new operator crashes when running the native tests, I will look at that.

We can also see 2 different flakiness behaviors here:

on Action triggers only while in custom state fails because one of the counters is 2 instead of 1, this looks like it might be an actual bug in the implementation or how the test is implemented
2 different tests failed once because they were already in the second state when we were expecting the initial state, I think this might be a test specific race condition. I'll check if my is assumption is right tomorrow.

sockeqwe · 2021-08-04T10:16:52Z

.../commonMain/kotlin/com/freeletics/flowredux/dsl/internal/OnActionInStateSideEffectBuilder.kt

@@ -76,5 +76,5 @@ class OnActionInStateSideEffectBuilder<InputState : S, S : Any, A : Any>(
 }


-typealias OnActionBlock<InputState, S, A> = suspend (action: A, state: InputState) -> ChangeState<S>
+typealias OnActionHandler<InputState, S, A> = suspend (action: A, state: InputState) -> ChangeState<S>


sockeqwe · 2021-08-04T10:17:03Z

...c/commonMain/kotlin/com/freeletics/flowredux/dsl/internal/OnEnterInStateSideEffectBuilder.kt

@@ -55,5 +55,5 @@ class OnEnterInStateSideEffectBuilder<InputState : S, S : Any, A : Any>(
    }
 }

-typealias InStateOnEnterBlock<InputState, S> = suspend (state: InputState) -> ChangeState<S>
+typealias InStateOnEnterHandler<InputState, S> = suspend (state: InputState) -> ChangeState<S>


sockeqwe · 2021-08-04T10:40:06Z

dsl/src/commonMain/kotlin/com/freeletics/flowredux/dsl/InStateBuilderBlock.kt

@@ -78,4 +164,4 @@ class InStateBuilderBlock<InputState : S, S : Any, A : Any>(
    }
 }

-typealias InStateObserverBlock<T, InputState, S> = suspend (value: T, state: InputState) -> ChangeState<S>
+typealias InStateObserverHandler<T, InputState, S> = suspend (value: T, state: InputState) -> ChangeState<S>


👍 i like the name handler much more!

sockeqwe · 2021-08-04T10:45:07Z

...monMain/kotlin/com/freeletics/flowredux/dsl/internal/CollectStateInStateSideEffectBuilder.kt

@@ -0,0 +1,60 @@
+package com.freeletics.flowredux.dsl.internal


Nitpicking: I find the class name confusing 😄

Not sure what a better name is and how to differentiate it from the other CollectInStateSideEffectBuilder ...

Maybe CollectAndTransformStateFlowInStateSideEffectBuilder? (that's a mouthful, but something like that?)

How does CollectInStateBasedOnStateSideEffectBuilder sound?

Sounds good to me 👍

sockeqwe · 2021-08-04T10:54:52Z

...monMain/kotlin/com/freeletics/flowredux/dsl/internal/CollectStateInStateSideEffectBuilder.kt

+    override fun generateSideEffect(): SideEffect<S, Action<S, A>> {
+        return { actions: Flow<Action<S, A>>, getState: GetState<S> ->
+            actions.whileInState(isInState, getState) { inStateActions ->
+                inStateActions.map { getState() as InputState }


Hm, that lines of code is hard to read for me (mentally I mean) because it requires knowledge that FlowRedux (inherited from RxRedux) is using the concept of SideEffects and the loopBack of each action to every SideEffect in the core flowredux library and that the loopback is triggered after each action went through the reducer....

So while it works, I was wondering if instead of subscribing to the Actions directly collect the StateFlow from FlowReduxStateMachine?

Then at least it would be nicer to read and maintain, otherwise as a Maintainer of the library I always need to have the implementation details of FlowRedux in mind.

Alternatively (and I think that would be more feasible) I would probably recommend to extract inStateActions.map { getState() as InputState } to something like fun Flow<Action<S,A>>.getCurrentStateFlow(getState : GetState<S>) = this.map { getState() as InputState }.distinctUntilChanged() so that "implementation details" of how to get to such a flow that emits current state is hidden from the reader of this piece of code.

What do you think?

So while it works, I was wondering if instead of subscribing to the Actions directly collect the StateFlow from FlowReduxStateMachine?

We can't do that ourselves because we don't have the StateFlow available here. A user could do it with the existing collectWhileInState, but I think doing that is weird and you need to cast yourself.

Extracting parts of this into a function to make readability better makes sense

9abaac9 this should improve the readability and also adds some comments to the extracted function.

sockeqwe · 2021-08-04T11:06:00Z

dsl/src/commonTest/kotlin/com/freeletics/flowredux/dsl/CollectStateWhileInStateTest.kt

+import kotlinx.coroutines.flow.flatMapConcat
+
+@OptIn(ExperimentalCoroutinesApi::class, FlowPreview::class, ExperimentalTime::class)
+class CollectStateWhileInStateTest {


Nitpicking: I'm wondering if we should merge this tests with the other CollectWhileInStateTest to only have one "collectWhileInState" file with all tests of all variations...

OR

if we should have better naming conventions (i.e. class under test with "Test" suffix, i.e. then CollectInStateSideEffectBuilderTest ...

I lean more towards the first option (merge all inState<S> { collectWhileInState(...) } into 1 file ...

I'm fine with merging 👍

sockeqwe · 2021-08-04T11:09:16Z

dsl/src/commonMain/kotlin/com/freeletics/flowredux/dsl/InStateBuilderBlock.kt

    ) {
+        on(FlatMapPolicy.LATEST, handler)
+    }


sockeqwe · 2021-08-04T11:10:35Z

..._code/src/commonMain/kotlin/com/freeletics/flowredux/sample/shared/PaginationStateMachine.kt

            }

            inState<ShowContentAndLoadingNextPageErrorPaginationState> {
-                onEnter(block = ::moveToContentStateAfter3Seconds)
+                onEnter(::moveToContentStateAfter3Seconds)
            }
        }
    }


Much better to readability 🎉

sockeqwe · 2021-08-04T11:15:48Z

dsl/src/commonMain/kotlin/com/freeletics/flowredux/dsl/InStateBuilderBlock.kt

+        handler: InStateObserverHandler<T, InputState, S>
+    ) {
+        collectWhileInState(flow, FlatMapPolicy.CONCAT, handler)
+    }


sockeqwe · 2021-08-04T11:32:08Z

I could also think about a more specific use case and therefore a convenience method would be nice where the orifinal state that triggered collectWhileInstate is passed as a parameter but it should not invoke again again with newer state updates.

Something of this method signature:

fun collectWhileInState(  
        flowBuilder: (InputState) -> Flow<T>, // so not a Flow<InputState>  but just `InputState` and only triggers once with the inputState that caused collectWhileInState to start
        handler: InStateObserverHandler<T, InputState, S>
)

i.e. if you want to observe a database for 1 particular Column / item by primary key, with SQLDelight or Room you don't want to cancel collection of query and start again because of flatMap transformation or so.

inState<ShowItemState> {
   collectWhileInState({ state : ShowItemState -> db.queryById(state.item.id) ) { item, state -> 
       MutateState { copy(item = item)  // So this triggers multiple times whenever the Item  in the database gets updated
    }
}

Another use case I could think of is connecting to a websocket when you would like to start only once the websocket connection.

Under the hood it could be just a resuing what you are about to introduce with the PR:

val stateFlow : Flow<InputState> = ...

stateFlow.take(1).flatMapLatest { state -> flowBuilder(state) } ...

Do you think something like that could be useful? I'm happy to introduce something like this as a follow up PR ...

…n-state

sockeqwe

Thank you so much @gabrielittner for working on this stuff 🙇‍♂️

gabrielittner · 2021-08-04T16:53:23Z

Have you considered making it an extension function on Flow<Action<S, A>>? For me, personally, having one parameter less (actions) improves readability a bit (not a game changer, thus up to you).

The reason why I didn't make it an extension is that I thought having a currentStateFlow method that gets some parameters makes more sense logically then transforming the actions Flow into a state Flow with an operator. If you just look at the point where it's called. From the implementation perspective it's still the same obviously.

I don't have a strong opinion here, so feel free to change it.

currentStateFlow(): In my head I still think about kotlin's StateFlow. No need to change, but maybe flowForCurrentState() or statemachinesStateAsFlow() ... Again, no need to change, can be merged as it is.

Updated to flowOfCurrentState 👍

sockeqwe · 2021-08-04T16:54:15Z

👍 🚢

add collectWhileInState functions with flow builders

58ab060

gabrielittner requested a review from sockeqwe August 3, 2021 20:27

update sample

552cd72

sockeqwe mentioned this pull request Aug 4, 2021

make FlowReduxStateMachine lazy #183

Merged

sockeqwe requested changes Aug 4, 2021

View reviewed changes

sockeqwe reviewed Aug 4, 2021

View reviewed changes

gabrielittner added 3 commits August 4, 2021 16:10

Merge remote-tracking branch 'origin/main' into collect-state-while-i…

a880e36

…n-state

extract some code into separate functions

9abaac9

merge collectWhileInState tests

a3b7e45

sockeqwe approved these changes Aug 4, 2021

View reviewed changes

gabrielittner added 3 commits August 4, 2021 18:48

update naming

033c533

update naming

304af77

update naming

86f7b31

gabrielittner added 2 commits August 4, 2021 18:55

also update usage of renamed builder

47c16c4

also remove old import

ff91b62

gabrielittner merged commit cb391ce into main Aug 4, 2021

gabrielittner deleted the collect-state-while-in-state branch August 4, 2021 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add collectWhileInState functions with flow builders #182

add collectWhileInState functions with flow builders #182

gabrielittner commented Aug 3, 2021

gabrielittner commented Aug 3, 2021

sockeqwe Aug 4, 2021

sockeqwe Aug 4, 2021

sockeqwe Aug 4, 2021

sockeqwe Aug 4, 2021

gabrielittner Aug 4, 2021

sockeqwe Aug 4, 2021

sockeqwe Aug 4, 2021

gabrielittner Aug 4, 2021

gabrielittner Aug 4, 2021

sockeqwe Aug 4, 2021

gabrielittner Aug 4, 2021

gabrielittner Aug 4, 2021

sockeqwe Aug 4, 2021

sockeqwe Aug 4, 2021

sockeqwe Aug 4, 2021

sockeqwe commented Aug 4, 2021

sockeqwe left a comment

gabrielittner commented Aug 4, 2021

sockeqwe commented Aug 4, 2021

		@@ -0,0 +1,60 @@
		package com.freeletics.flowredux.dsl.internal

add collectWhileInState functions with flow builders #182

add collectWhileInState functions with flow builders #182

Conversation

gabrielittner commented Aug 3, 2021

gabrielittner commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sockeqwe commented Aug 4, 2021

sockeqwe left a comment

Choose a reason for hiding this comment

gabrielittner commented Aug 4, 2021

sockeqwe commented Aug 4, 2021