GitLiveApp / firebase-kotlin-sdk

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

Moves ValueWithSerializer back into public API #605

Closed Daeda88 closed 4 weeks ago

Daeda88 commented 1 month ago

It had become part of commons-internal which kind of defeats the purpose.

Also made FirebaseEncoder/FirebaseDecoder an interface in the public API. Useful for writing custom Serializers that have custom behaviour on Firebase

nbransby commented 1 month ago

It had become part of commons-internal which kind of defeats the purpose.

I'm going to need more of an explanation, is it to fix this issue https://github.com/GitLiveApp/firebase-kotlin-sdk/issues/590 ?

Daeda88 commented 1 month ago

No I actually ran into it myself. The ValueWithSerializer class was originally in the regular commons library and allows encoding a value when dealing with File paths. But it's no longer accessible so I moved it back. Though I'm not entirely sure if this was introduced by Splendo or GitLive. Technically you can achieve the same behavior with a custom SerializerModule though, this does add some convenience

nbransby commented 4 weeks ago

allows encoding a value when dealing with File paths

Could you give a code example of what you mean? A search of ValueWithSerializer in the README doesn't return any results.

mr-kew commented 4 weeks ago

@Daeda88 Does this somehow relate to #604?

Daeda88 commented 4 weeks ago

Its mostly about:

public data class DocumentReference {
    public suspend inline fun update(vararg fieldsAndValues: Pair<String, Any?>, buildSettings: EncodeSettings.Builder.() -> Unit = {})
}

where the Any in the pair is encoded using dev.gitlive.firebase.firestore.encode(it, buildSettings), which means you cannot set a custom serializer for it.

But I see it was introduced by Splendo in #448 and if that is so, I actually think we should solve it a different way more consistent with the rest of the library: by adding support for passing the serializer to the update methods. I'll change this PR to reflect that and ping you when it's done.

Daeda88 commented 4 weeks ago

Dropping in favour of #607