GitLiveApp / firebase-kotlin-sdk

A Kotlin-first SDK for Firebase
https://gitliveapp.github.io/firebase-kotlin-sdk/
Apache License 2.0
1.16k stars 156 forks source link

Open the library for additional serialization approaches #38

Closed brezinajn closed 3 years ago

brezinajn commented 4 years ago

It would be a good idea to at least expose additional API to be able to plug other serialization providers. Now we have

fun <T> WriteBatch.set(documentRef: DocumentReference, strategy: SerializationStrategy<T>, data: T, merge: Boolean): WriteBatch
fun <T> DocumentSnapshot.data(strategy: DeserializationStrategy<T>): T

Proposed API

inline fun <T> WriteBatch.set(documentRef: DocumentReference, crossinline encode: (T)->Any?, data: T, merge: Boolean): WriteBatch
inline fun <T> DocumentSnapshot.data(crossinline parse: (Any?)->T): T

By providing this API we should be able to plug in any serialization library (including kotlinx.Serialization).

teamhubuser commented 4 years ago

Hi @brezinajn what serialization provider are you using?

brezinajn commented 4 years ago

When it's my decision kotlinx.Serialization. But in some older projects we have both Moshi and Jackson. We even have some projects using Helios by 47degrees. The thing is just to provide ability to plug serialization solution that suits the needs of your project.

nbransby commented 4 years ago

Problem with such a change is what format is the data passed to the parse function in? If we pass the raw format from underlying Firebase library it will be platform specific

brezinajn commented 4 years ago

Problem with such a change is what format is the data passed to the parse function in? If we pass the raw format from underlying Firebase library it will be platform specific

That is a good point. It should be possible in the same manner as with current serializer/deserializer implementation.

fun <T> dev.gitlive.firebase.decode(strategy: DeserializationStrategy<T>, value: Any?): T {
    require(value != null || strategy.descriptor.isNullable) { "Value was null for non-nullable type ${strategy.descriptor.serialName}" }
    return FirebaseDecoder(value).decode(strategy)
}

What this function does is, it takes (multiplatform) deserializer and some value that came from firestore and runs the value through deserializer. There should be no need to force a specific deserializer at this point.

fun <T> decode(parse: (Any?)->T, value: Any?): T = parse(value)

fun <T> decode(strategy: DeserializationStrategy<T>, value: Any?): T = parse({ data ->
    require(value != null || strategy.descriptor.isNullable) { "Value was null for non-nullable type ${strategy.descriptor.serialName}" }
    FirebaseDecoder(data).decode(strategy)
}, value)

In reality this is the only change needed.

nbransby commented 4 years ago

Well that doesn't solve the issue I mentioned, the Any? passed in the parse function will be different things depending on which platform you are running on and we shouldn't expose that to clients.

Perhaps what we could do is add a singleton serializer which you can pass in which would return the platform specific version which the client can then manage the deserialization of. That way we don't pollute the API with low level functionality

brezinajn commented 4 years ago

Oh I see where is the difference. I think about this as very low level API of "Don't impose unnecessary constraints and let the user handle it" type. If you don't wanna expose this to the user, the singleton approach seems fine.

jonatbergn commented 3 years ago

I'm also puzzled on how one could decode firstore's (platform specific) GeoPoint and Timestamp values. Is that even possible given the current design?

nbransby commented 3 years ago

@jonatbergn solution is https://github.com/GitLiveApp/firebase-kotlin-sdk/issues/64#issuecomment-662481715

nbransby commented 3 years ago

closing due to lack of demand