rjaros / kvision

Object oriented web framework for Kotlin/JS
https://kvision.io
MIT License
1.24k stars 65 forks source link

Redux states - do they need to be serializable? #58

Closed robert-cronin closed 5 years ago

robert-cronin commented 5 years ago

Is there any specific reason why KVision's redux module should require serializable data classes? I'm running into a problem with a custom class of ObjectID from MongoDB. I've written a custom serializer for my class:

@Serializer(forClass = ObjectID::class)
object ObjectIDSerializer: KSerializer<ObjectID> {
    override val descriptor: SerialDescriptor =
            StringDescriptor.withName("ObjectID")

    override fun serialize(encoder: Encoder, obj: ObjectID) {
        encoder.encodeString(obj.toString())
    }

    override fun deserialize(decoder: Decoder): ObjectID {
        return ObjectID(decoder.decodeString())
    }
}

but when I try to run it in the browser, I get Can't locate argument-less serializer for class null even though none of my variables in the redux state are nullable. Is it possible to remove the requirement for it to be serializable? Unless there is a specific reason why it needs to be.

robert-cronin commented 5 years ago

I've just created a redux implementation that doesn't require data classes to be serializable, it is based on this webpage: https://jayrambhia.com/blog/kotlin-redux-architecture Here is the code for anyone who would like to use non-serializable data classes in redux:

interface RAction
typealias Reducer<State> = (State, RAction) -> State
typealias Subscription<State> = (State) -> Unit

interface StoreInterface<State> {
    fun getState(): State
    fun subscribe(subscription: Subscription<State>)
    fun unsubscribe(subscription: Subscription<State>)
}

class Store<State>(
        initialState: State,
        private val reducers: List<Reducer<State>>): StoreInterface<State> {

    private var currentState: State = initialState
    private val subscriptions = arrayListOf<Subscription<State>>()

    override fun getState() = currentState

    private fun applyReducers(current: State, RAction: RAction): State {
        var newState = current
        for (reducer in reducers) {
            newState = reducer(newState, RAction)
        }
        return newState
    }

    internal fun dispatch(RAction: RAction) {
        val newState = applyReducers(currentState, RAction)
        if (newState == currentState) {
            // No change in state
            return
        }
        currentState = newState
        subscriptions.forEach { it(currentState) }
    }

    override fun subscribe(subscription: Subscription<State>) {
        subscriptions.add(subscription)
        // notify subscription of the current state
        subscription(currentState)
    }

    override fun unsubscribe(subscription: Subscription<State>) {
        subscriptions.remove(subscription)
    }
}

fun <S> createReduxStore(initialState: S, reducers: List<Reducer<S>>): Store<S> {
    return Store(initialState, reducers)
}

@rjaros I can replace the existing kvision redux module with this one if you think its appropriate, the usage can be made to be similar to the existing lighteningkite dependency?

robert-cronin commented 5 years ago

I also have trouble with the Tabulator module requiring serializable classes, is there any particular reason why Tabulator requires these as well? if it didn't, that would solve a big problem for me

robert-cronin commented 5 years ago

I was able to get Tabulator working without serialization as well, it now just depends on the new redux module, shall I commit both of those?

rjaros commented 5 years ago

First the Redux case. It's not that simple. The original Redux (redux.js.org) is quite sophisticated piece of software. It's extendable with addons and middleware and kvision-redux module includes one of the most widely used ones: redux-thunk. It allows to dispatch "Action creators" functions, which is necessary for any asynchronous operations. Your redux implementation is very basic and has no support for this. With original Redux implementation you can use any middleware you want, and the Redux ecosystem gives you very large choice. That's the reason I've integrated KVision with original Redux library. I'm watching some Kotlin projects (https://github.com/freeletics/CoRedux, https://github.com/gumil/Kaskade), but none of them are targeting Kotlin/JS yet.

Regarding the serializable requirement - the kotlin-redux project I'm using has some limitations with Kotlin Lists. I've decided that support for Lists is more important, so my kvision-redux module serializes the state to the JSON string before passing it to Redux. This way we can use any classes for the Kotlin state, as long as they are serializable - this is a trade-off.

What can be done:

  1. We can easily create second kvision-redux module without serialization requirement. But you will not be able to use Lists in your state (and should be ready for other incompatibilities).
  2. We can create yet another redux module with your implementantion instead of the original. But you will not be able to dispatch action creators or use any other Redux addons.
rjaros commented 5 years ago

Regarding Tabulator serializable requirement. It's not really a requirement. You can use Tabulator with any class you want (even Any) - but you will not be able to use some sophisticated functionality, which depends on the dataSerializer existence (kotlin reactive data model, some custom editing events, custom editors, formatters and filters).

rjaros commented 5 years ago

Without serializable data model, you just have to use native JS Tabulator functionality - you have to work with native JS data and not Kotlin data.

rjaros commented 5 years ago

And this time I don't really see any workarounds. Without dataSerializer there is no way to turn internal Tabulator data model into Kotlin data model, so these function just can't work.

robert-cronin commented 5 years ago

Well either I'm missing something, or we really don't need serializable (for Redux at least), I've just rewritten the redux implementation in KVision to remove serialization and it works just fine with my complicated State and Reducers. I have multiple classes (e.g. ObjectID from MongoDB) without custom serializers that can be called from the state inside the subscribe function. I've also tested this simple example given in the guide for thunk middleware:

store.dispatch { dispatch, getState ->
    window.setTimeout({
        dispatch(MyAction.Increment)
    }, 1000)
}

I think this can be explained because the JetBrains redux wrapper that KVision depends on does not appear to restrict classes to being serializable, the restriction seems to come directly from the KVision code.

As for the limitation with lists, could we perhaps find a way around this, perhaps using Arrays instead of lists like they suggest?

robert-cronin commented 5 years ago

Am I missing something? I can upload the new code if you want to investigate it further

rjaros commented 5 years ago

The description of the restriction actually mentions using the combineReducer function. And KVision doesn't use this function. I'll check it out, because you can be right - maybe we don't need to serialize anything.

robert-cronin commented 5 years ago

yes I hope so, removing serialization would simplify a lot of my code, currently I have to have two classes, one that operates with KStitch that doesn't require serialization and one that operates with KVision that does. I will upload the replacement code shortly for you to check out.

robert-cronin commented 5 years ago

here are the modified files in redux main: main-redux.zip let me know what you think 👍

robert-cronin commented 5 years ago

I can put this in a pull request alongside the tabulator update, I've found the same thing applicable to the Tabulator module, an ObservableList as a Tabulator data source works just fine without the @Serializable annotation.

rjaros commented 5 years ago

The redux module is already fixed in my source tree. I've modified some tests to check if lists are working and it seems they do. Thx for this enhancement.

robert-cronin commented 5 years ago

ok no problems, I could have submitted a PR but that's okay. I will send through a PR for the Tabulator update.

rjaros commented 5 years ago

But I'm still not convinced about the Tabulator - have you modified the Tabulator class and all the options classes by removing the whole dataSerializer dependent code? I have no idea how it can work without it :-) Please, create PR and I'll check it out.

robert-cronin commented 5 years ago

I've just submitted a PR, perhaps there are some obscure options I have not considered, but all of the options I am using seem pretty obscure already (FormatterComponentFunction, ObservableLists). Let me know if you spot anything that is pretty obviously in need of a serializer 👍

Edit: the code is combined with the Pace module PR, I still haven't found a way to have two separate PRs from the same fork

rjaros commented 5 years ago

I had some issues with custom editors, but I've managed to make them work. I'm still not sure this change is 100% safe (it will probably work correctly with data classes only), but I think it's worth trying.

robert-cronin commented 5 years ago

that's good news, if there is something fundamental about the serializable code we can always change it back but I don't think so.

I always thought Kotlin data classes got compiled into named JavaScript objects. It appears that way if you run the below code:

data class TestClass(val someVar1: String, val someVar2: String)
...
{
    val testClass = TestClass("1234","4321")
    console.log(testClass) // TestClass {someVar1: "1234", someVar2: "4321"}
    console.log(jsTypeOf(testClass)) // object

    val testObject = jsObject {
        someVar1 = "1234"
        someVar2 = "4321"
    }
    console.log(testObject) // {someVar1: "1234", someVar2: "4321"}
    console.log(jsTypeOf(testObject)) // object
}

I don't think there is ever a need to serialize Kotlin classes to work with external JavaScript libraries.

Anyway, I think removing the serializable requirement makes it less restrictive overall 👍