beyondeye / Reduks

A "batteries included" port of Reduxjs for Kotlin+Android
Apache License 2.0
111 stars 10 forks source link

Add phantom Action as a phantom type for all action #5

Closed kittinunf closed 8 years ago

kittinunf commented 8 years ago

Thanks so much for this library!

How do you feel about having a phantom type for all action? I personally feel that the usage of Object for an action is too broad. This could be a simple and empty interface that provide a loose frame for all user action to follow.

interface Action

Another thing on my mind right now is having a Standard action for Reduks. It is pretty much similar to "upstream" redux.js counterpart where they define a recommended-but-not-mandatory structure for Action. You could look at here for the reference, flux-standard-action. This could be something similar to this

interface StandardAction {
   val payload: Any?
   val error: Boolean 
   val meta: Any?
}

What do you think?

beyondeye commented 8 years ago

I was planning to add standard action. There is actually an implementation in kotlin by Michael Pardo that I would like to adopt

kittinunf commented 8 years ago

Sounds great. Because I ended up with my "standard"-ish interface for all of my actions anyway. Would love to see this bundle in the Reduks core.

beyondeye commented 8 years ago

About a base interface for action, I am afraid about boilerplate and overhead. What are the use cases you are thinking about?

kittinunf commented 8 years ago

You meant the phantom type action or the standard action?

beyondeye commented 8 years ago

The phantom type

kittinunf commented 8 years ago

It is a phantom type so, just for compiler to check. This is for an invalid action not being able to represent in code.

If there was a phantom type action, I could write something like this;

object Foo : Action
object Bar : Action

//usage
store.dispatch(Foo) //compiler allow

But not this

object Foo 
class Bar

store.dispatch(Foo) //error as Foo does not implement Action
store.dispatch(Bar()) // same here
beyondeye commented 8 years ago

What about a store extension method dispatchWithcheck or something similar?

beyondeye commented 8 years ago

And still allow dispatch without check as default?

beyondeye commented 8 years ago

Or better a store enhancer

kittinunf commented 8 years ago

I am not a fan of having yet another extension.

For me as we are in a strong-type language, type info is almost always a good thing to enforce it. If it were my call, I would rather have the check bundled by default. Every place in core that we use Any to represent Action should be substituted with Action. This is to make intention clear.

I will leave the ultimate decision to you. Boilerplate and overhead will be there definitely. But I think it is worth.

beyondeye commented 8 years ago

I think you are right. Probably this need to be linked with introduction of the standard action you were talking about. I will start to work on a draft implementation. Also if you want to write a draft of what you want to see in a standard action you are welcome

beyondeye commented 8 years ago

this is the code I was talking about that I want to adopt (perhaps with some changes) for the standard action format https://github.com/pardom/redux-standard-action-kotlin/blob/master/lib/src/main/kotlin/redux/fsa/Action.kt

beyondeye commented 8 years ago

it is a port of https://github.com/acdlite/flux-standard-action and https://github.com/acdlite/redux-actions

beyondeye commented 8 years ago

A side note about flux standard action. Using a string for identifying the action type is not necessarily good for performance, at least when on a java8 JVM see http://stackoverflow.com/questions/103564/the-performance-impact-of-using-instanceof-in-java

kittinunf commented 8 years ago

I think redux.js uses String as identifier because there is no type information in JavaScript. On the other hand, Kotlin does support sealed class with when so, identifying type is much easier. Personally, for FSA we could just go for payload and error.

On Fri, Aug 26, 2016, 7:26 PM beyondeye notifications@github.com wrote:

A side about flux standard action. Using a string for identifying the action type is not necessarily good for performance, at least when on a java8 JVM see http://stackoverflow.com/questions/103564/the-performance-impact-of-using-instanceof-in-java

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/beyondeye/Reduks/issues/5#issuecomment-242713264, or mute the thread https://github.com/notifications/unsubscribe-auth/AEdATeay8udncJicNflLHGp01CoagwpAks5qjtKkgaJpZM4Jt3Nw .

kittinunf commented 8 years ago

Regarding to Pardom's implementation, it looks okay to me. But as I prefer, I do not think that type and meta should be enforced hence having both of them as generic type is a bit too much.

beyondeye commented 8 years ago

I agree about Pardom implementation. probably better going with something simpler. By the way. What are you using the meta field for, in your code?

beyondeye commented 8 years ago

what about something like this?

interface Action

fun StandardAction.started():Boolean {
    return payload==null && !error
}
fun StandardAction.completed():Boolean {
    return payload!=null && !error
}
fun StandardAction.isError():Boolean {
    return error
}
interface StandardAction:Action {
    val payload: Any?
    val error: Boolean
    val meta: Any?
    class StandardActionMatcher<PayloadType>(val action:StandardAction) {
         fun onStarted(body: () -> Unit): StandardActionMatcher<PayloadType> {
            if(action.started())
                body()
            return this
        }
        fun onCompleted(body: (value: PayloadType) -> Unit): StandardActionMatcher<PayloadType> {
            if(action.completed())
                body(action.payload as PayloadType)
            return this
        }
        fun onFailed(body: (error: Throwable) -> Unit): StandardActionMatcher<PayloadType> {
            if(action.error)
                body(action.error as Throwable)
            return this
        }
    }
    companion object {
        inline fun <reified PayloadType:Any > withPayload(action: Any,meta:Any?=null): StandardActionMatcher<PayloadType>? {
            if(action !is StandardAction) return null
            if(action.payload !is PayloadType) return null
            meta?.let {if(it !=action.meta) return null }
            return StandardActionMatcher(action)
        }
    }
}
kittinunf commented 8 years ago

I rarely use it to be honest. However, there is one scenario I think it could prove to be very useful is undoing optimistic update.

Let's say you are doing optimistic update. And later on your server tells you that the update you have made on client is error / not possible. You undo action could be

interface FSA {
    val payload: Any?
    val error: Boolean
}

class AddTodoAction(override val payload: Int, override val error: Boolean, val meta: Int) : FSA

store.dispatch(AddToDoAction(payload = ..., error = true, meta = 123)

Then, we can undo our update with data from meta. In the mentioned case, 123 is the id of todo we need to remove from the list so that undo could be taken place.

beyondeye commented 8 years ago

or perhaps better like this

interface Action{
    companion object {
        inline fun <reified PayloadType:Any > withPayload(action: Any,meta:Any?=null): StandardActionMatcher<PayloadType>? {
            if(action !is StandardAction) return null
            if(action.payload !is PayloadType) return null
            meta?.let {if(it !=action.meta) return null }
            return StandardActionMatcher(action)
        }
    }
}
interface StandardAction:Action {
    val payload: Any?
    val error: Boolean
    val meta: Any?
}

fun StandardAction.started():Boolean {
    return payload==null && !error
}
fun StandardAction.completed():Boolean {
    return payload!=null && !error
}
class StandardActionMatcher<PayloadType>(val action:StandardAction) {
    fun onStarted(body: () -> Unit): StandardActionMatcher<PayloadType> {
        if(action.started())
            body()
        return this
    }
    fun onCompleted(body: (value: PayloadType) -> Unit): StandardActionMatcher<PayloadType> {
        if(action.completed())
            body(action.payload as PayloadType)
        return this
    }
    fun onFailed(body: (error: Throwable) -> Unit): StandardActionMatcher<PayloadType> {
        if(action.error)
            body(action.error as Throwable)
        return this
    }
}
kittinunf commented 8 years ago

I might be a bit too naiive but I don't see the need of StandardActionMatcher. What actually does StandardActionMatcher use for?

martin-g commented 8 years ago

if(action.error) body(action.error as Throwable)

How a Boolean would be casted to a Throwable ?

beyondeye commented 8 years ago

I will explain what I am trying to do. In brief to make it easier to write reducer. A standard action can wrap several sub action types , and the logic for their treatment does not really depends on their specific type. you need to handle started() (when an async action start) error, and success. It is not really necessary but can make writing the reducer easier.

Boolean cast to Throwable: sorry it is a mistake, I meant

body(action.payload as Throwable)
beyondeye commented 8 years ago

@kittinunf can you share an example on how you usually write reducers when using FSA?

kittinunf commented 8 years ago

Sure.

Normally, I tend to stick with sealed class as much as possible.

Take "todolist" as pedagogical example.

State;

data class Todo(val id: Int, val text: String)
data class TodoState(val todos: List<Todo> = listOf())

Action;

sealed class TodoAction(override val payload: Any? = null, 
                        override val error: Boolean = false) : FSA {
  class Init() : TodoAction()
  class Add(override val payload: Map<String, Any>) : TodoAction()
  class Remove(override val payload: Int) : TodoAction()
}

Reducer;

fun todoReducer() = Reducer<TodoState> { state, action ->
  when (action) {
    is TodoAction.Init -> TodoState()
    is TodoAction.Add -> {
        val payload = action.payload
        val todo = Todo(payload["id"] as Int, payload["text"] as String)
        val newTodos = state.todos.toMutableList()
        newTodos.add(todo)
        state.copy(todos = newTodos)
    }
    is TodoAction.Remove -> {
        val newTodos = state.todos.filter { it.id == action.payload }
        state.copy(todos = newTodos)
    }
    else -> { throw IllegalStateException() }
  }
}

Good thing about FSA is a communication between programmers. Clearly define what payload, error, meta are makes everyone on the same page and resonates among other ppl's code.

Might not cover everything, but it is mostly for you to get the idea.

beyondeye commented 8 years ago

I agree with you, One last thing. How do you handle FSA with error ( in reducer): with some chain of IFs on the value of error, and/or check if payload is null, or with something more structured?

kittinunf commented 8 years ago

Yeah, having another chain of if could be messy. Also, checking payload introduces some sort of if anyway so it might not be ideal.

In my project, I end up with something really simple. At root reducer, I compose 2 sub-reducers into a big reducer. When action is fired, it branches off to "success" reducer automatically for optimistic update. Later, if it found out that error needs to be handled. Same type of action with error flag on is fired, then it branches off to "failure" reducer to perform undo. So, there is 2 working implementation for each and every action that has possibility of failure. One to "do" stuff, and another to "undo".

As my project is small, this tactic is working quite well. I am not sure about larger project though. So please take it for a grain of salt.

beyondeye commented 8 years ago

Ok. Thanks a lot for your suggestions. I will create a branch for phantom action, and I will try to commit all the required changes in the next few days. I will let review it before merging it to master

beyondeye commented 8 years ago

Questions about preferred class name for Standard Action. 1) FSA 2) KSA 3) FSAction 4) StandardAction 5)other

kittinunf commented 8 years ago

I would say using abbreviation is unconventional in Kotlin world. So, I vote for 4.

martin-g commented 8 years ago

I also prefer 4).

beyondeye commented 8 years ago

done. you can look at changes in phantom action branch

beyondeye commented 8 years ago

issue solved in release 2.0.0b5