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

Open the library for additional serialization approaches #38

Closed
brezinajn opened this issue Jun 12, 2020 · 9 comments
Closed

Open the library for additional serialization approaches #38

brezinajn opened this issue Jun 12, 2020 · 9 comments

Comments

@brezinajn
Copy link

brezinajn commented Jun 12, 2020

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

Hi @brezinajn what serialization provider are you using?

@brezinajn
Copy link
Author

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

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

brezinajn commented Jun 16, 2020

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

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

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

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

@jonatbergn solution is #64 (comment)

@nbransby
Copy link
Member

closing due to lack of demand

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

4 participants