Refactor & optimize some Kotlin Coroutines & Flow usage

[luongvo]

Issue

We applied using Kotlin Coroutines & Flow in our template and client projects for a while. During that time, there is a room for improvement with more excellent extensions, better approaches, or shorter callers. We should consider refactoring & optimizing them for our templates.

Solution

In this RFC scope, I would like to raise some points to apply:

• Using launchIn instead of collect for collecting Flows.

          logInUseCase(
              email = email,
              password = password
          )
              .onStart { showLoading() }
              .onCompletion { hideLoading() }
              .catch { e -> _error.emit(e) }
              .onEach {
                  _isLoggedIn.emit(true)
                  navigateToHome()
              }
              .launchIn(viewModelScope)
  

  References:

  • https://itnext.io/differences-in-methods-of-collecting-kotlin-flows-3d1d4efd1c2?gi=9a000a4781f1
  • https://handstandsam.com/2021/02/19/the-best-way-to-collect-a-flow-in-kotlin-launchin/

• Defining some valuable extensions for Compose to reduce boilerplate code

  • collectAsEffect

    @Composable
    fun <T> Flow<T>.collectAsEffect(
        context: CoroutineContext = EmptyCoroutineContext,
        block: (T) -> Unit
    ) {
        LaunchedEffect(key1 = Unit) {
            onEach(block).flowOn(context).launchIn(this)
        }
    }
    

    viewModel.navigator.collectAsEffect { destination -> navigator(destination) }
    

  • References:

    • https://github.com/elson-dsouza/PopMovies/blob/dce98cd63540e6c88b8913a978c78edaf2fe035e/app/src/main/java/com/example/elson/popmovies/compose/EventsSupport.kt
    • https://github.com/igamgam97/MovieDB/blob/a2cd3e0dd171115c9d422f96944e0b19a41cee95/core/common/src/main/java/com/noveogroup/moviecatalog/core/common/FlowExt.kt
    • https://github.com/Cilestal/Kotlin-Multiplatform-MVI-sample-Android-Compose-iOS-SwiftUI-/blob/7e68ec688ef7c0685a883a708c06454fe60d8d0c/androidApp/src/main/java/com/cilestal/mvi_mpp/android/extension/ComposeExt.kt
    • https://github.com/adiRandom/grocever-android/blob/eab84e8a6294a7ad429a51f5353620d35e6b033b/app/src/main/java/app/adi_random/dealscraper/usecase/FlowUtils.kt
    • https://github.com/adiRandom/grocever-android/blob/39fc41cd46ef0f6497b289059359c3bb56e1b0c3/app/src/main/java/app/adi_random/dealscraper/usecase/FlowUtils.kt

• Refactor flowTransform extension to use runCatching and get rid of try/catch with generic exception caught.

  @OptIn(ExperimentalTypeInference::class)
  fun <T> flowTransform(@BuilderInference block: suspend FlowCollector<T>.() -> T) = flow {
      runCatching { block() }
          .onSuccess { emit(it) }
          .onFailure { throw it.mapError() }
  }
  

Who Benefits?

Developers

What's Next?

• Preparing tickets for improvement
• Making contributions to the Compose template. We can consider applying for the XML template if needed.

[lydiasama]

@luongvo I agree for using launchIn and refactoring flowTransform. But I'm not sure about collectAsEffect extension.

For this implementation:

@Composable
fun <T> Flow<T>.collectAsEffect(
    context: CoroutineContext = EmptyCoroutineContext,
    block: (T) -> Unit
) {
    LaunchedEffect(key1 = Unit) {
        onEach(block).flowOn(context).launchIn(this)
    }
}

key1 would be fixed to Unit every time. But from my understanding, these keys are for observing changes and are used for doing the LaunchedEffect when there are some changes. By doing this, I am concerned that it would not be good for performance. 🤔 What do you think?

[lydiasama]

Or perhaps we could custom it more like this:

@Composable
fun <T> Flow<T>.collectAsEffect(
    key: Any? = Unit,
    context: CoroutineContext = EmptyCoroutineContext,
    block: (T) -> Unit
) {
    LaunchedEffect(key1 = key) {
        onEach(block).flowOn(context).launchIn(this)
    }
}

[kaungkhantsoe]

@lydiasama I thought so too. That's why I initially suggested including the key at the PR. Apparently, it is not the case. It works with the Unit too. If we run LaunchedEffect with the Unit, the block should run only once per composition. Why the block here is running on every emit is because of launchIn.

1. Because LaunchedEffect is run with Unit, the flow is being collected outside of the composition.
2. When we emit the new value to the flow, LaunchedEffect is not called again but launchIn is keep collecting the new value from the flow.

LaunchedEffect is originally used for calling suspend functions. But since the scope of the LaunchedEffect is in CoroutineScope we can collect the flow too. So I must say the function is running correctly as out of the box.

You can see in the attached recording that when a new value is emitted to the flow, debug point only goes to the collect scope, it doesn't relaunch the effect. I used a simple approach like collect instead of launchIn for demonstration purpose. 🙏 cc @Wadeewee , @hoangnguyen92dn

Screen.Recording.2023-02-22.at.3.17.23.PM.mp4

[hoangnguyen92dn]

@kaungkhantsoe As I discussed with Lucas about the purpose of the extension. It makes sense since we wrap the launchIn with the LaunchedEffect(Unit) for the coroutine scope, it doesn't suppose about recomposition when the state changes. I think we could add the explain when could we use the extension to avoid confusion @luongvo. cc @lydiasama @Wadeewee

[hoangnguyen92dn]

@luongvo It's good to bring these improvements to the team in the discussion. There are 3 different topics so quite inconvenience when we have to point out which one we agree and disagree IMO 😄

Using launchIn instead of collect for collecting Flows.

Agree with this. This is convenient when we need to collect events from Flow, especially in the ViewModel layer 👍

collectAsEffect

I think @lydiasama's concern is valid, the key should allows type as Any rather than fixed as Unit

Refactor flowTransform extension to use runCatching and get rid of try/catch with generic exception caught.

No more concern from me, as I remember we discussed this before.

[Wadeewee]

I agreed with @lydiasama concern for collectAsEffect. We also have this concern in the Gulf project as well. For these points 👇

Using launchIn instead of collect for collecting Flows.

Refactor flowTransform extension to use runCatching and get rid of try/catch with generic exception caught.

All good to me 👍

[kaungkhantsoe]

FYI: We can update the collectAsEffect as described in the suggestion to be able to call suspend function inside the block. cc @luongvo

[luongvo]

@lydiasama @hoangnguyen92dn @kaungkhantsoe @Wadeewee I think you all agreed to have some extensions to optimize our Kotlin Coroutines & Flow usage. Let's not go too deep into the implementation detail in this RFC and discuss more in the implementation PR instead.

What do you think? If yes, please vote for the RFC, and I'll start creating tasks for this. Many thanks 🤗

[Wadeewee]

Voted 🚀

[luongvo]

@ryan-conway I think I received enough votes. More discussions would be raised in the final implementation PRs 💪

I'm going to create 3 separate issues for this.

[luongvo]

I created 3 separate tasks from this RFC:
#425
#426
#427

[toby-thanathip]

@luongvo Cool! Voted 🚀

Will the extension function for Compose be added to com.github.nimblehq:android-common-kt ? 💭

cc @hoangnguyen92dn

[luongvo]

@Tuubz @hoangnguyen92dn it requires Compose API, so at least we need to separate it into a separate lib as an option instead, like

• com.github.nimblehq:android-common-ktx
• com.github.nimblehq:android-common-compose-ktx

That is a separate action from my task 🤷‍♂️

[toby-thanathip]

@luongvo Got it, thank you 🚀

I'll lock the discussion 🔒