badoo / MVIKotlin

Extendable MVI framework for Kotlin Multiplatform with powerful debugging tools (logging and time travel), inspired by Badoo MVICore library
https://arkivanov.github.io/MVIKotlin
Apache License 2.0
826 stars 66 forks source link

Better support for executor creating actions #209

Closed dalewking closed 3 years ago

dalewking commented 3 years ago

I know that Actions are there for bootstrapped but I find the desire to have the executor itself be able to initiate actions. There can be multiple reasons to do that. One might be that you have common core handling an action that might also need to make occur when handling an intent and it is more convenient to just send the same action. It can also just be a better way to organize code into named units of work. I see the difference between action and intent is just that intents are externally visble and actions are internal.

The library already does support the ability to do this because there is nothing preventing code in handle intent from calling handleAction. While this does work it is bypassing code. Most notably if you have logging those actions won't be logged.

So what I think I am asking for is for the Callbacks to be extended with an onAction method that can be used in the Executor to fire actions.

With that addition I'm not sure if bootstrapped serves much purpose as you can just override init and fire Actions there

arkivanov commented 3 years ago

Hi @dalewking, thanks for raising this and for the use cases provided.

The original idea was to handle all additional actions internally in the Executor. E.g. you can extract the Action handling into separate private functions, which can be called at any point of time internally by the Executor. But as you mentioned, it is kinda "bypassing". So I find your idea worth to consider. I will need to think about how it fits from multiple perspectives: breaking API changes, logging, time travelling, etc. I will provide updates here.

Worth to mention that there is a workaround to dispatch Actions from Executors:

class MyStoreFactory(
    private val storeFactory: StoreFactory
) {

    fun create(): MyStore {
        val bootstrapper = MySimpleBootstrapper<Action>(Action.Init)

        return object : MyStore, Store<Intent, State, Nothing> by storeFactory.create(
            name = "MyStore",
            initialState = State(),
            bootstrapper = bootstrapper,
            executorFactory = { ExecutorImpl(bootstrapper::sendAction) },
            reducer = ReducerImpl
        ) {}
    }

    private sealed class Action {
        object Init : Action()
        object DoSomething : Action()
    }

    private sealed class Result

    private class ExecutorImpl(
        private val action: (Action) -> Unit
    ) : SuspendExecutor<Intent, Action, State, Result, Nothing>() {
        override suspend fun executeAction(action: Action, getState: () -> State) {
            //
        }

        override suspend fun executeIntent(intent: Intent, getState: () -> State) {
            action(Action.DoSomething)
        }
    }

    private object ReducerImpl : Reducer<State, Result> {
        override fun State.reduce(result: Result): State {
            TODO("Not yet implemented")
        }
    }
}

class MySimpleBootstrapper<Action : Any>(
    private vararg val actions: Action
) : SuspendBootstrapper<Action>() {
    override suspend fun bootstrap() {
        actions.forEach(::dispatch)
    }

    fun sendAction(action: Action) {
        dispatch(action)
    }
}
dalewking commented 3 years ago

Yes, I am aware that I could hack it that way to go through the Bootstrapper, but it kind of points out why the design is questionable.

The way I see it. If Executor can initiate Actions itself there is no need for Bootstrapper to even exist. Bootstrapping would then just be done in the Executor in response to the init call.

I should let you know that over the weekend I ported the basic Store functionality to Dart for use on a Flutter project and it is working. I hope to eventually clean it up and publish it. I want it to be able to hook up to the Time travel plugin for the IDE, but I haven't worked on the time travel stuff. Because I am writing it anew I am not constrained exactly to the same design, so I think I will eliminate Bootstrapper in my Dart implementation.

arkivanov commented 3 years ago

Thanks for the input. Are you actually using the library or just porting it?

dalewking commented 3 years ago

porting it

dalewking commented 3 years ago

Here is a link to a dump of my dart version. No documentation or comments: https://gitlab.com/dalewking/dart_mvi

arkivanov commented 3 years ago

Here is a link to a dump of my dart version. No documentation or comments: https://gitlab.com/dalewking/dart_mvi

Thanks! Glad to see the library is being ported to other languages!

arkivanov commented 3 years ago

I made a decition to not add this. At the moment the Executor has two inputs (Intent and Action) and two outputs (Result and Label). Making Action as the Executor's input and output will add confusion and complexity to the internal Store system without significant benefits.

I recomment to just extract shared code to private functions:

interface SomeStore : Store<Intent, State, Nothing> {
    sealed class Intent {
        data class DeleteItem(val id: String) : Intent()
        data class CheckItem(val id: String) : Intent()
    }

    class State
}

class SomeStoreFactory {
    private sealed class Action {
        data class DeleteItem(val id: String): Action()
    }

    private sealed class Result

    private class ExecutorImpl : ReaktiveExecutor<Intent, Action, State, Result, Nothing>() {
        override fun executeIntent(intent: Intent, getState: () -> State) {
            when (intent) {
                is Intent.DeleteItem -> deleteItem(id = intent.id)
                is Intent.CheckItem -> checkItem(id = intent.id)
            }
        }

        override fun executeAction(action: Action, getState: () -> State) {
            when (action) {
                is Action.DeleteItem -> deleteItem(id = action.id)
            }
        }

        private fun deleteItem(id: String) {
            // Delete the item
        }

        private fun checkItem(id: String) {
            if (/*some condition*/) {
                deleteItem(id = id)
            }
        }
    }
}

As of pointlessness of the Bootstrapper in case of the Executor producing Actions directly. The init(callbacks) method is final by design, and so can not be overridden. We would have to add another init method without arguments. The additional init method and the Executor being responsible for producing initial Actions would add additional responsibility to the Executor. This would also break the event pipeline, because the Executor could produce Results or Labels directly from the init method, so an output is produced without any input. So even with the ability of the Executor to produce Actions directly, I would still keep the Bootstrapper as it is.