Suggestion to improve API for CoroutineExecutor

[ultraon]

Hello Arkadii, thanks for this great library. I have a suggestion to improve the CoroutineExecutor contract, literally, it simplifies the usage of explicit scope property.

open class CoroutineExecutor<in Intent : Any, in Action : Any, in State : Any, Result : Any, Label : Any>(
    protected val scope: CoroutineScope = MainScope()  // <--------- ADDED
) : Executor<Intent, Action, State, Result, Label> {

    private val callbacks = atomic<Callbacks<State, Result, Label>>()
    private val getState: () -> State = { callbacks.requireValue().state }
    // protected val scope: CoroutineScope <------- REMOVED

//...
}

Then we can use CoroutineExecutor in this way:

class MyCoroutineExecutor(
  scope: CoroutineScope = MainScope() + CoroutineName("MyCoroutineExecutor")
) : CoroutineExecutor<...>(scope), CoroutineScope by scope {

    private fun processIntentX(intent: IntentX) {
         launch {  // <------------ HERE WE DON'T HAVE WRITE `scope.launch {}`
              val result = withContext(Dispatchers.IO) {
                   // heavy work
              }
              dispatch(ResultX(result))
         }
    }

// ...
}

To do that with existing contract I should:

class MyCoroutineExecutor(
  context = Dispatchers.Main + CoroutineName("MyCoroutineExecutor")
) : CoroutineExecutor<...>(context), CoroutineScope {

    override val coroutineContext: CoroutineContext
        get() = scope.coroutineContext

    private fun processIntentX(intent: IntentX) {
         launch {  // <------------ HERE WE DON'T HAVE WRITE `scope.launch {}`
              val result = withContext(Dispatchers.IO) {
                   // heavy work
              }
              dispatch(ResultX(result))
         }
    }

// ...
}

[arkivanov]

Hello @ultraon and thanks for the idea!

First of all I would like to stress, that providing the CoroutineScope as a property was an explicit design decision. Making custom classes to implement CoroutineScope is discouraged, according to docs, various messages in Slack and some articles: Docs page, A slack thread, An article. The main reason is that it's error prone, you my accidentally launch a coroutine in incorrect scope.

Passing a scope via constructor - currently I find this also not a good API. CoroutineExecutor closes the scope on dispose. Currently CoroutineScope is encapsulated (owned and controlled by the CoroutineExecutor), and so its lifetime is guaranteed to be between CorutineExecutor instantiation and disposal. Passing CoroutineScope via constructor removes ownership from CoroutineExecutor.

If you still really won't to implement CoroutineScope, then maybe you can create your own base class?

open class BaseExcecutor(...) : CoroutineExecutor(...), CoroutineScope by scope {
...
}

[ultraon]

It’s a pretty good explanation, thank you.