uniflow-kt / uniflow-kt Goto Github PK

Hi!

With the stable version I'm facing a recurring issue when navigating away from a screen. It's the most common crash among my users so far.

kotlinx.coroutines.channels.ClosedSendChannelException: Channel was closed
        at kotlinx.coroutines.channels.Closed.getSendException(AbstractChannel.kt:1106)
        at kotlinx.coroutines.channels.AbstractSendChannel.helpCloseAndResumeWithSendException(AbstractChannel.kt:210)
        at kotlinx.coroutines.channels.AbstractSendChannel.access$helpCloseAndResumeWithSendException(AbstractChannel.kt:19)
        at kotlinx.coroutines.channels.AbstractSendChannel.sendSuspend(AbstractChannel.kt:200)
        at kotlinx.coroutines.channels.AbstractSendChannel.send(AbstractChannel.kt:137)
        at kotlinx.coroutines.channels.ChannelCoroutine.send$suspendImpl(Unknown Source:2)
        at kotlinx.coroutines.channels.ChannelCoroutine.send(Unknown Source:0)
        at io.uniflow.core.flow.ActionReducer.enqueueAction(ActionReducer.kt:36)
        at io.uniflow.core.flow.ActionDispatcher$actionOn$2.invokeSuspend(ActionDispatcher.kt:40)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

Seems like with the new engine it'll go away so it could be great to say goodbye to this awful bug.

Another option would be to actually publish the beta version In order to be able to use it since it's not available even it appears in the documentation.

Thanks!

Originally posted by @erikhuizinga in #12 (comment)

Side note for a different issue:
Why are state actions offered to the actor instead of using send? offer returns false and the action isn't added to the queue, but send suspends the sender until the buffer has capacity again. Why is the latter not an option?

Imagine I'm setting a loading state:

0. setState { LoadingState } -> UI shows loading UI
1. DataFlow loads data -> data loaded
2. setState { DataState } <- oops! The buffer was already full, so the new state was never actually set.
3. Now the UI still is in the loading state, but nothing is loading.

If send would be used instead of offer, the setState { DataState } would eventually set the correct state. This probably has other issues / challenges and maybe you've already considered this. WDYT?

Hi All 👋

the "old" Android support package will be deprecated for end of year. I propose to merge both uniflow-android & uniflow-androidx packages.

Which name do we keep then?

• uniflow-android
• uniflow-androidx

I would propose to keep uniflow-android naming, and merge the AndroidX package inside.

Feature Request
This library does a great job logging STATE and EVENT updates, making it much easier to debug app issues. One improvement to this can be to log all UI input events that occur. In my apps I typically handle all input from the UI in the ViewModel layer with a single function called processInput. A sealed class type is passed as a parameter to tell the ViewModel what type of UI event occurred. This is also useful for app analytics as it can be handled all in one place.

Potential Solution
One solution to this could be to create an optional open function in the AndroidDataFlow class to log all UI input events that occur. I'm sure there is a better way, but just my initial thought on it.

This is an awesome library! Keep up the great work.

I think I have encountered a bug.

When using onEvents and consuming in two fragments. The event only fires in the first Fragment as both fragments have the same consumer ID LiveDataPublisher I think this consumer ID should be the name of the Fragment but I'm not sure.

Hello all,

since a while I wondering if a multiplatform version can be a good idea. I believe that such tool will help build app until UI layers, letting us choose the right native framework after that: Android or iOS.

From the internal dev side, uniflow is already discoupled in terms of implementation. Allowing an Android impl. Core needs to be ported as a Kotlin MP project then.

But more in a 2.0 version, right now the 1.0 needs to be out.

Hi all,

I'm working on Jetpack compose for Koin. I believe Uniflow would be a good companion for Compose, as it's direct in link to unidirectional flow :)

Hi!

I think would be a good idea to use something like distinctUntilChanged() in order to avoid repeated states so it keeps away from drawing over and over unnecessarily.

I'm using this library in production and found that sometimes the re-draw is unnecessary since the state hasn't changed at all.

Hi, It's my first time using MVI and i used your library to try and implement it. While doing so i noticed that you can't have a running action and also call other actions at the same time. This happened to me because i want to use a flow from room database as single source of truth and send other actions to update data from api and save to room, or do other stuff. I don't understand if this is a bug or if it's by design, but if it is by design is there no way in MVI to have an ongoing subscription to db data?

Hi @arnaudgiuliani, thanks a lot for this library, I was thinking it would be awesome to have a package for TornadoFX to target JavaFX / Desktop app.

I can see that it's commented out on master.
What are the blockers for releasing it?
Do you need any help with the implementation?

AndroidDataFlow.<init> sets the default state asynchronously:

However, a common use case for me is to extend AndroidDataFlow and in my implementation's init block I also call action { setState { someStateThatILoaded } }. These two actions are concurrently launched in ActionDispatcher:

Remember that CoroutineScope.launch {} returns quickly. However, AFAIK no order of execution is guaranteed! If the scope is backed by multiple threads, the order of these launched jobs is undefined and therefore 🐛 it is undefined which of the two states is set first!

User story

As a user I expect the defaultState constructor argument to be set as soon as all AndroidDataFlow<init> blocks return.

Suggestion

Do not asynchronously set state in AndroidDataFlow.<init>, but instead synchronously set state.

Workaround

Await defaultState and only then start setting your own states. In other words: don't set state from your AndroidDataFlow implementation's init block.

Version affected

UniFlow version: 0.11.2

Right now we are checking the only state that is coming from vararg testingData: UIData to improve the quality of tests maybe we should also check the size of this list?

fun verifySequence(vararg testingData: UIData) {
        val testingValues = testingData.toList()
        values.forEachIndexed { index, uiData ->
            assertEquals(uiData, testingValues[index],
                    "Wrong values at [$index] - expecting: ${uiData::class.simpleName}"
            )
        }
    }
    
    
  

Use case:

TestDispatchersRule uses Dispatchers.Unconfined. A user writing tests might use a TestCoroutineDispatcher to test their coroutines. If used together with TestDispatchersRule, the user now is using two different coroutine dispatchers. This might lead to undetected leaking coroutines or to difficult to debug errors.

This is just a hypothesis. I have no working example that shows that there can(not) be problems. Please let me know what you think that could happen when using both Dispatchers.Unconfined and TestCoroutineDispatcher in tests.

If there is a particular reason not to use TestCoroutineDispatcher or to only use Dispatchers.Unconfined, let me know, as I'm unaware of it.

AFAIK TestCoroutineDispatcher gives more control over the coroutines, but it also introduces a bit of boilerplate:

• The Main dispatcher must be replaced using Dispatchers.setMain() before each test.
  • It must also be reset after each test using Dispatchers.resetMain()
• Test functions using coroutines should be wrapped in runBlockingTest (not required, but recommendable).
• After each test cleanupTestCoroutines() should be called on the TestCoroutineDispatcher.

Similarly, there is TestCoroutineScope that behaves similarly to TestCoroutineDispatcher.

Relevant code:

Suggestion:

• Rename TestDispatchersRule to UnconfinedDispatchersRule, rename TestDispatchers to UnconfinedDispatchers. This keeps the implementation around as I'm sure it has applications.
• Use TestCoroutineDispatcher in TestDispatchersRule and TestDispatchers. Also add the necessary before/after steps to be taken. Update the documentation to suggest the use of TestDispatchersRule and runBlockingTest.

Hi,

another proposition is to stop using LiveData for StateFlow. Would avoid such Event<> UIevent wrapper

Restore state doesn't update state value stored in LiveDataPublisher.

PersistentLiveDataPublisher.restoreState updates _states object but _currentState in LiveDataPublisher never gets updated on restore.

This means states.value will have the correct value in your view model but getState() will be incorrect.

I only saw the states but not the events. What should I do?

With Uniflow 0.11.4 when I setState with a message only:

setState(UIState.Failed("Some message here")

I get a fatal exception:

Fatal Exception: kotlin.TypeCastException
null cannot be cast to non-null type io.uniflow.core.flow.data.UIError
io.uniflow.core.flow.data.UIState$Failed.<init>

I am pretty sure it's be cause of this line where null is being cast.

Hello,

I think that Arrow extensions should be in a separated module.
Arrow dependency should not be included when we use uniflow-core nor uniflow-android(x).

TLDR:

I would love to write assertions like this:

expectThat(view.state).isEqualTo(SomeState(param1, param2))

but the current implemenation of MockedViewObserver prevents me from doing so.

The Problem

When testing the state of a viewmodel it is sometimes difficult to know which part of the state is incorrect and because MockedViewObserver only exposes hasState and hasEvent Below is an example of the issues I am currently encountering. Given the following test:

    @Before
    fun setup() {
        viewModel = SubmitIDStateViewModel()
        view = viewModel.mockObservers()
    }

    @Test
    fun `should mark state as valid if ID is valid`() {
        private val fakeAddress = Faker.address.build()

        viewModel.updateAddress(fakeAddress)

        coVerify {
            view.hasState(SubmitIDState(address= fakeAddress, id= ID(), isValid= true))
    }

This test fails because isValid never becomes true, but that is very unclear based on the error output:

java.lang.AssertionError: Verification failed: call 1 of 1: Observer(#3).onChanged(eq(SubmitIDState(address=Address(street=21 jump st), id=ID(), isValid=true)))). No matching calls found.

Calls to same method:
1) Observer(#3).onChanged(SubmitIDState(address=Address(street=), id=ID(id=default), isValid=false))
2) Observer(#3).onChanged(SubmitIDState(address=Address(street=21 jump st), id=ID(id=default), isValid=false))
3) Observer(#3).onChanged(SubmitIDState(address=Address(street=21 jump st), id=ID(id=), isValid=false))

One could argue this is a problem with Mockk and the way the verify method handles parameter assertions, but the way MockedViewObserver is implemented feels tightly coupled to the Mockk implementation because it only exposes the hasState method and does not allow for other assertion libraries or approaches to validating internal data on the state object. This is why I would like for the state to be exposed in MockedViewObserver so I could write assertions like this:

    expectThat(view.state).isEqual(SubmitIDState(address= fakeAddress, id= ID() , isValid = true))

Could be useful for side effects like analytics.

implementation "io.uniflow:uniflow-saferesult:$version" is missing from any documentation, but it is required to use the SafeResult feature.

Hi I Wold Like to hide state print , there is a possibility ?

Actually, when we use actionOn<> everything is working correctly but I notice one issue. We can't use it base sealed class

This is where we are calling this method and comparing it

action.targetState?.let { targetState ->
                if (targetState != currentState::class) {
                    action.onError(BadOrWrongStateException(currentState, targetState), currentState)
                    return
                }
            }

Do we want to improve this ?

It's an issue about LiveDataPublisher, notifyStateUpdate function can't work with current implementation as it's always publishing state. We need an intermediate backing field for state, as it as in "old" version.

Hello,

with the Jcenter sunsetting in a few time, we need to update publication scripts to help publish on maven central.

Hello, I am trying to update to 1.1.1 but it looks like the aar isn't published on Maven: https://repo.maven.apache.org/maven2/org/uniflow-kt/uniflow-android/1.1.1/

UniFlow 1.0.10 calls Action.onError twice for error functions that throw an exception. The default onError function for data flows throws an exception, defined here: https://github.com/uniflow-kt/uniflow-kt/blob/1.0.10/uniflow-core/src/main/kotlin/io/uniflow/core/flow/DataFlow.kt#L42-L45. So, the default behaviour is that the error function is run twice.

Likely, this is caused by actionOn<T : UIState> {} being reduced by https://github.com/uniflow-kt/uniflow-kt/blob/1.0.10/uniflow-core/src/main/kotlin/io/uniflow/core/flow/ActionReducer.kt#L50-L62. This code calls action.onError twice, because:

1. The state isn't the target state: 👍.
2. The BadOrWrongStateException is thrown by the first onError call, which is caught by catch (e: Exception) which runs onError again: 👎.

The MWE below simply tests that the default onError function is called twice, because the default function logs a message. The test simply counts the logged error messages. There should be exactly one, but there are two. This is by no means a good test for this issue, but it simply demonstrates the issue.

MWE (deps: JUnit 4.13.2, UniFlow 1.0.10):

import io.uniflow.android.AndroidDataFlow
import io.uniflow.core.flow.actionOn
import io.uniflow.core.flow.data.UIEvent
import io.uniflow.core.flow.data.UIState
import io.uniflow.core.logger.Logger
import io.uniflow.core.logger.UniFlowLogger
import io.uniflow.test.rule.UniflowTestDispatchersRule
import kotlin.test.assertEquals
import kotlinx.coroutines.test.TestCoroutineDispatcher
import org.junit.Rule
import org.junit.Test

class SingleLogTest {
    companion object {
        private val testCoroutineDispatcher = TestCoroutineDispatcher()
    }

    @get:Rule
    val uniflowTestDispatchersRule = UniflowTestDispatchersRule(testCoroutineDispatcher)

    @Test
    fun `bad or wrong state is logged once`() {
        val logger = object : Logger {
            val messages = mutableListOf<String>()

            override fun debug(message: String) {
            }

            override fun log(message: String) {
            }

            override fun logError(errorMessage: String, error: Exception?) {
                messages += errorMessage
                println(errorMessage)
            }

            override fun logEvent(event: UIEvent) {
            }

            override fun logState(state: UIState) {
            }
        }

        UniFlowLogger.init(logger)

        val androidDataFlow = object : AndroidDataFlow(defaultState = UIState.Empty) {
            fun foo() = actionOn<UIState.Loading> {}
        }

        androidDataFlow.foo() // Should log an uncaught error about a BadOrWrongStateException

        assertEquals(1, logger.messages.size)
    }
}

Fails with

expected:<1> but was:<2>
Expected :1
Actual   :2

(Also reported through the Kotlin language Slack here.)

The testing docs (https://github.com/uniflow-kt/uniflow-kt/blob/master/doc/testing.md) suggest that it might be possible to test the default UIState of a DataFlow. In fact, on UniFlow 0.11.6 the following test passes, but on v1.0.10 it fails! This is unexpected and possibly even a bug! The suggestion is to also emit the default state for testing purposes, as this is likely what the user intends to test.

MWE:

// Imports skipped for brevity, and because they differ for v0.11.6 and v1.0.10

class ADFTest {
    @get:Rule
    val instantTaskExecutorRule = InstantTaskExecutorRule()

    @Test
    fun androidDataFlowEmitsDefaultState() {
        val androidDataFlow = object : AndroidDataFlow(defaultState = UIState.Success) {}
        val testObserver = androidDataFlow.createTestObserver()
        assertEquals(1, testObserver.statesCount)
    }
}

Hello,

the fact that Throwable is not comparable as is in Kotlin is a big problem. It forces us to make some workaround to make it "comparable".

If we check what is a comparable:

/**
 * The base class for all errors and exceptions. Only instances of this class can be thrown or caught.
 *
 * @param message the detail message string.
 * @param cause the cause of this throwable.
 */
public open class Throwable(open val message: String?, open val cause: Throwable?) {
    constructor(message: String?) : this(message, null)

    constructor(cause: Throwable?) : this(cause?.toString(), cause)

    constructor() : this(null, null)
}

I would propose then a data class type to encapsulate Throwable:

open class ThrowableKt(val message: String? = null, val cause: ThrowableKt? = null) {

    constructor(message: String? = null) : this(message, null as? ThrowableKt)
    constructor(message: String? = null, cause: Throwable? = null) : this(message, cause?.toThrowableKt())
    constructor(message: String? = null, cause: Exception? = null) : this(message, cause?.toThrowableKt())

    override fun equals(other: Any?): Boolean {
        if (this === other) return true
        if (javaClass != other?.javaClass) return false

        other as ThrowableKt

        if (message != other.message) return false
        if (cause != other.cause) return false

        return true
    }

    override fun hashCode(): Int {
        var result = message?.hashCode() ?: 0
        result = 42 * result + (cause?.hashCode() ?: 0)
        return result
    }

    override fun toString(): String = "ThrowableKt(message='$message',cause=$cause)"
}

fun Throwable.toThrowableKt(): ThrowableKt {
    return ThrowableKt(message, cause?.toThrowableKt())
}

fun throwableKt(t: Throwable) = t.toThrowableKt()

What do you think?

Hey!

I've found an issue that state don't get updated when you try to update the state from a flow onEach method which have another flow concatenated afterwards. Or even crashing when you merge (using flatmapMerge or flatmapLatest - which run in parallel).

java.lang.IllegalStateException: Flow invariant is violated:
		Emission from another coroutine is detected.
		Child of "coroutine#6":ProducerCoroutine{Active}@82c57b3, expected child of "coroutine#1":UndispatchedCoroutine{Active}@5be82d43.
		FlowCollector is not thread-safe and concurrent emissions are prohibited.
		To mitigate this restriction please use 'channelFlow' builder instead of 'flow'

To be honest I haven't checked the internal code and don't know which are your expectations. However, is a really bad experience not being able to update the state when executing two flows or more

Thanks!

Hello there,

Uniflow 1.1.0 has been pushed, bringing last fixes & lib updates.

Cheers.

I think a good feature for this library would be a restore state system using SavedStateHandle in order to avoid state loss when the process get killed. It can be done manually but I think it reduces the user's worry about it.

Any thoughts?

The following code prevents calling setState within setState.

Bad code:

// In an AndroidDataFlow

fun foo() {
  setState {
    setState { SomeState } // <- Error!
    AnotherState
  }
}

However, I am allowed to do this:

// In an AndroidDataFlow

fun foo() {
  setState { // Outer setState
    bar()
    AnotherState
  }
}

fun bar() {
  setState { SomeState } // Inner setState
}

The two examples are not equivalent, because the first erroneous setState is a member of StateAction (the receiver inside the first lambda), and the setState in bar is a member of DataFlow. The latter is allowed to be called anywhere on a DataFlow. The problem is that they seem equivalent and nothing prevents me from doing this.

I wonder if the behaviour is well-defined in my second example. The outer setState will run on a coroutine that will update the internal state, but before it does, a new action from the inner setState call is scheduled on that same coroutine? When will what action execute? Will I first observe AnotherState and then SomeState or the other way around?

For a UniFlow v1 release it might be nice to clean up the code base by requiring a certain style, coding conventions and/or formatting. Tools like detekt and ktlint (the former is built on the latter) can help. Detekt comes with a Gradle plugin and there is a Gradle plugin for ktlint, for simple usage on every platform that can run Gradle tasks, e.g CLI and CI.

Hey

I have a proposal to switch to using sealed classes, data classes and objects to represent intents.

Currently we are exposing multiple functions from our view models to our views to represent actions

class MyLoginViewModel : AndroidDataFlow()
  fun login(mobileNumber: String, code: String) = action { 
    // Manipulate state
  }
  
  fun resendCode(mobileNumber: String) = action { 
   // Manipulate state
  }
}

// In our view
viewModel.login(mobileNumber, code)
viewModel.resendCode(mobileNumber)

Recently in projects I have been using this approach to have fewer public functions and use sealed classes for intents and handle the intents in an exhaustive way.

sealed class MyLoginIntent {
  data class Login(mobileNumber: String, code: String) : MyLoginIntent()
  data class ResendCode(mobileNumber: String) : MyLoginIntent()
}

class MyLoginViewModel : AndroidDataFlow()
  fun onAction(intent: MyLoginIntent) = action {
    when(intent) {
      is MyLoginIntent.Login -> login(intent)
      is MyLoginIntent.ResendCode -> resendCode(intent)
    }  
  }
  
  private suspend fun login(loginIntent: MyLoginIntent.Login) { 
     // Manipulate state
  }

  private suspend fun resendCode(resendCodeIntent: MyLoginIntent.ResendCode) {  
     // Manipulate state
  }
}

// In our view
viewModel.onAction(MyLoginIntent.Login(mobileNumber, code)
viewModel.onAction(MyLoginIntent.ResendCode(mobileNumber)

Currently Uniflow is taking care of manipulating and representing state in an exhaustive self documenting way. It is taking care of the flow of state but not really taking care of the flow of intents. What do you think of making this pattern part of Uniflow in some way.

When debugging test code stack traces it helps if you give custom names to mocks.

Currently (Uniflow 0.9.0 with AndroidX), a stack trace might look like this:

java.lang.AssertionError: Verification failed: calls are not exactly matching verification sequence

Matchers: 
Observer(#1).onChanged(eq(...)))

But since the user does not use Observer instances or the Observer class (not directly, at least), this can be confusing.

In the code below you don't give custom names to the MockK mocks you create.

Suggestion: give custom names to these mocks (e.g. "Uniflow UIState Observer mock" and "Uniflow UIEvent Observer mock"), or make them settable through an optional argument to mockObservers for additional flexibility.

The result would still contain Observers, but would then become something like:

java.lang.AssertionError: Verification failed: calls are not exactly matching verification sequence

Matchers: 
Observer(Uniflow UIState Observer mock#1).onChanged(eq(...)))

Here the responsible code:

I can make a PR if you want.