Serialization improvements #312
Conversation
| @@ -9,15 +9,15 @@ import kotlinx.serialization.descriptors.PolymorphicKind | |||
| import kotlinx.serialization.descriptors.SerialDescriptor | |||
| import kotlinx.serialization.descriptors.StructureKind | |||
|
|
|||
| actual fun FirebaseDecoder.structureDecoder(descriptor: SerialDescriptor, decodeDouble: (value: Any?) -> Double?): CompositeDecoder = when(descriptor.kind) { | |||
There was a problem hiding this comment.
special value handling is replaced by using a custom serializer
| * A special case of serializer for values natively supported by Firebase and | ||
| * don't require an additional encoding/decoding. | ||
| */ | ||
| abstract class SpecialValueSerializer<T>( |
There was a problem hiding this comment.
in this PR it's used for Realtime Database ServerValue and Firestore FieldValue and Timestamp. Firestore also supports storing GeoPoints and DocumentReferences natively, but it's out of scope of this PR
|
|
||
| /** Represents a Firebase ServerValue. */ | ||
| @Serializable(with = ServerValueSerializer::class) | ||
| actual class ServerValue internal actual constructor( |
There was a problem hiding this comment.
using a wrapper class for ServerValue, FieldValue and Timestamp allows specifying a custom serializer and also ensures correct equality checks in JS
| android.takeUnless { fieldsAndValues.isEmpty() } | ||
| ?.update( | ||
| documentRef.android, | ||
| fieldsAndValues[0].first, |
There was a problem hiding this comment.
a bugfix here. the first value was never encoded
| ) | ||
|
|
||
| /** A serializer for a Double field which is stored as a Timestamp. */ | ||
| object DoubleAsTimestampSerializer : SpecialValueSerializer<Double>( |
There was a problem hiding this comment.
A serializer which allows storing a timestamp in a double field. potentially useful for a legacy behaviour
| @@ -112,10 +99,16 @@ actual class WriteBatch(val ios: FIRWriteBatch) { | |||
| ios.updateData(encode(strategy, data, encodeDefaults) as Map<Any?, *>, documentRef.ios).let { this } | |||
|
|
|||
| actual fun update(documentRef: DocumentReference, vararg fieldsAndValues: Pair<String, Any?>) = | |||
| ios.updateData(fieldsAndValues.associate { it }, documentRef.ios).let { this } | |||
| ios.updateData( | |||
| fieldsAndValues.associate { (field, value) -> field to encode(value, true) }, | |||
There was a problem hiding this comment.
a bugfix here. it seems like iOS was not encoding values
| @@ -65,6 +65,42 @@ class FirebaseDatabaseTest { | |||
| assertEquals(3, firebaseDatabaseChildCount) | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
I'd appreciate some help on running JS tests for firebase-database module locally. there is a warning on jsLegacyTest run on current master
ReferenceError: $module$firebase_compat_app is not defined
at ..../firebase-database/src/jsMain/kotlin/dev/gitlive/firebase/database/database.kt:18:1
at Object.<anonymous> (..../build/js/packages/firebase-kotlin-sdk-firebase-database-js-legacy-test/kotlin/firebase-kotlin-sdk-firebase-database-js-legacy-test.js:1093:2)
and it seems like the tests aren't executed at all - no reports, no DB updates
vpodlesnyak
force-pushed
the
serialization-improvements
branch
3 times, most recently
from
36fbe8c to
0efe662
Compare
|
can you rebase and then we can re-review it |
# Conflicts: # firebase-database/src/androidMain/kotlin/dev/gitlive/firebase/database/database.kt # firebase-database/src/commonTest/kotlin/dev.gitlive.firebase.database/database.kt
|
@Reedyuk done |
|
Looks good, lets wait for the ci to pass and @nbransby can review too |
This pull request uses serializers API to simplify encoding / decoding and provides more type safe way to operate with Timestamps