Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
robert-cronin opened this issue Jun 21, 2019 · 19 comments
Closed

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

robert-cronin opened this issue Jun 21, 2019 · 19 comments

Comments

@robert-cronin
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

rjaros commented Jun 21, 2019

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
Copy link
Owner

rjaros commented Jun 21, 2019

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
Copy link
Owner

rjaros commented Jun 21, 2019

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
Copy link
Owner

rjaros commented Jun 21, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@rjaros
Copy link
Owner

rjaros commented Jun 21, 2019

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
Copy link
Contributor Author

robert-cronin commented Jun 21, 2019

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
Copy link
Contributor Author

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

@robert-cronin
Copy link
Contributor Author

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
Copy link
Owner

rjaros commented Jun 21, 2019

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
Copy link
Contributor Author

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

@rjaros
Copy link
Owner

rjaros commented Jun 21, 2019

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
Copy link
Contributor Author

robert-cronin commented Jun 21, 2019

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
Copy link
Owner

rjaros commented Jun 21, 2019

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
Copy link
Contributor Author

robert-cronin commented Jun 22, 2019

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 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants