Fix js + enable js tests + code clean #45

vpodlesnyak · 2022-05-13T07:38:51Z

the goal of this PR is to fix JS code and enable JS tests. what was done

created wrapper classes for GeoPoint, Timestamp and FieldValue. it allowed to specify serializers, so it could be used in documents without need of annotations. it also allowed to fix the equality check issues in JS.
Removed classes FirebaseTimestamp and FirebaseReference deprecated in the previous PR. deletion shall not be done the way those classes provided and a cleaner alternative is available for the Timestamp
fixed JS implementation issues
commonized some tests and added some more

challenges:

native api for FieldValue is different between platforms, so expect / actual declarations can't be used. so there is no compile time type check
native equality checks for FieldValue.arrayUnion and FieldValue.arrayRemove are not working

…se of JS limitation

vpodlesnyak · 2022-05-13T07:42:59Z

firebase-common/src/commonTest/kotlin/dev/gitlive/firebase/EncodersTest.kt

@@ -71,17 +71,19 @@ class EncodersTest {
 @Test
 fun testEncodeDecodedSealedClass() {
 val test = SealedClass.Test("Foo")
- val encoded = encode(test, false)
- val decoded = decode(encoded) as? SealedClass.Test
+ val serializer = SealedClass.Test.serializer() // has to be used because of JS issue


legacy JS is not finding a serializer in serializer<T>() call (see issue) and it seems unlikely to be fixed. the upstream repo is using legacy JS and not IR JS, so I don't consider migrating at the moment

firebase-common/src/androidMain/kotlin/dev/gitlive/firebase/_encoders.kt

vpodlesnyak · 2022-05-13T07:45:25Z

firebase-common/src/jsMain/kotlin/dev/gitlive/firebase/externals.kt

@@ -459,6 +461,8 @@ external object firebase {
 open class GeoPoint(latitude: Double, longitude: Double) {
 val latitude: Double
 val longitude: Double
+
+ fun isEqual(other: GeoPoint): Boolean


unfortunately interops doesn't map it to equals

vpodlesnyak · 2022-05-13T07:46:39Z

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/encoders.kt

@@ -22,17 +22,3 @@ internal fun <T> encode(strategy: SerializationStrategy<T>, value: T, shouldEnco
 shouldEncodeElementDefault,
 FieldValue.serverTimestamp()
 )
-


android and ios indeed encode data as map, however JS encodes it to json. updated platform specific code (as mentioned above)

vpodlesnyak · 2022-05-13T07:47:46Z

firebase-firestore/src/jsMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt

- val serializedItem = encodeAsMap(strategy, data, encodeDefaults)
- val serializedFieldAndValues = encodeAsMap(fieldsAndValues = fieldsAndValues)
-
- val result = serializedItem + (serializedFieldAndValues ?: emptyMap())


an implementation bug. it's not Map, but Json in JS

vpodlesnyak · 2022-05-13T07:49:11Z

firebase-firestore/src/jsMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt

@@ -257,7 +255,15 @@ actual class DocumentReference(val js: firebase.firestore.DocumentReference) {
 actual suspend fun update(vararg fieldsAndValues: Pair<String, Any?>) = rethrow {
 fieldsAndValues.takeUnless { fieldsAndValues.isEmpty() }
 ?.map { (field, value) -> field to encode(value, true) }
- ?.let { encoded -> js.update(encoded.toMap()) }


incorrect method was used for fields update. fixed by using the same approach update(vararg fieldsAndValues: Pair<FieldPath, Any?>) does

vpodlesnyak · 2022-05-16T13:48:53Z

firebase-firestore/src/androidAndroidTest/kotlin/dev/gitlive/firebase/firestore/firestore.kt

- }
-
- @Serializable
- data class TestDataWithDocumentReference(


moved to common tests

vpodlesnyak · 2022-05-16T13:50:30Z

firebase-firestore/src/androidMain/kotlin/dev/gitlive/firebase/firestore/GeoPoint.kt

+
+/** A class representing a Firebase GeoPoint. */
+@Serializable(with = GeoPointSerializer::class)
+actual class GeoPoint internal actual constructor(internal actual val platformValue: PlatformGeoPoint) {


wrapper class allows using @Serializable and overriding equals for JS

vpodlesnyak · 2022-05-16T13:55:09Z

firebase-firestore/src/androidMain/kotlin/dev/gitlive/firebase/firestore/Timestamp.kt

+@Serializable(with = TimestampSerializer::class)
+actual class Timestamp private constructor(
+ internal actual val platformValue: PlatformTimestamp,
+ actual val isServerTimestamp: Boolean


the approach with a boolean isServerTimestamp is not super clean, but I don't like other options either.. for example if a sealed class is used - we'd won't have a simple constructor..

vpodlesnyak · 2022-05-16T13:58:08Z

firebase-firestore/src/androidMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt

@@ -107,10 +107,12 @@ actual class WriteBatch(val android: com.google.firebase.firestore.WriteBatch) {
 merge: Boolean,
 vararg fieldsAndValues: Pair<String, Any?>
 ): WriteBatch {
- val serializedItem = encodeAsMap(strategy, data, encodeDefaults)
- val serializedFieldAndValues = encodeAsMap(fieldsAndValues = fieldsAndValues)


encodeAsMap can't be used as JS doesn't encode to a map so I scraped it and put implementation here directly

vpodlesnyak · 2022-05-16T14:00:46Z

firebase-firestore/src/androidMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt

- actual fun arrayRemove(vararg elements: Any): Any = FieldValue.arrayRemove(*elements)
- actual fun delete(): Any = delete
+/** A class representing a platform specific Firebase FieldValue. */
+internal typealias PlatformFieldValue = com.google.firebase.firestore.FieldValue


I wish I could use a common expect PlatformFieldValue however in Android/JS it's an abstract class and in iOS it's not. this causes modality is different error. any idea how I could trick the compiler?

vpodlesnyak · 2022-05-16T14:17:14Z

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/FieldValueSerializer.kt

+ if (encoder is FirebaseEncoder) {
+ encoder.value = value.platformValue
+ } else {
+ throw IllegalArgumentException("This serializer must be used with FirebaseEncoder")


it'd be nice to serialize it, but FieldValue.arrayUnion and arrayDelete take Any and I have no idea how to serialize it in a clean way.. use Json maybe?..

vpodlesnyak · 2022-05-16T14:24:11Z

firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/FieldValueTests.kt

+ assertNotEquals(FieldValue.delete, FieldValue.serverTimestamp())
+
+ // Note: arrayUnion and arrayRemove can't be checked due to vararg to array conversion
+// assertEquals(FieldValue.arrayUnion(1, 2, 3), FieldValue.arrayUnion(1, 2, 3))


any suggestions to have equality working as expected?

assertContentEquals?

the thing is that an array is stored in a private field and the firebase FieldValue class does not implement override equals as it should

Can you make Set out of Array?

vpodlesnyak · 2022-05-16T14:33:46Z

firebase-firestore/src/jsMain/kotlin/dev/gitlive/firebase/firestore/GeoPoint.kt

+ override fun equals(other: Any?): Boolean =
+ this === other || other is GeoPoint && platformValue.isEqual(other.platformValue)
+ override fun hashCode(): Int = platformValue.hashCode()
+ override fun toString(): String = "GeoPoint[lat=$latitude,long=$longitude]"


JS lib does not provide a nice toString hence implementing a custom representation

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/GeoPointSerializer.kt

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/Timestamp.kt

vpodlesnyak · 2022-05-18T08:15:07Z

firebase-firestore/src/androidMain/kotlin/dev/gitlive/firebase/firestore/Timestamp.kt

+
+/** A base class that could be used to combine [Timestamp] and [Timestamp.ServerTimestamp] in the same field. */
+@Serializable(with = BaseTimestampSerializer::class)
+actual sealed class BaseTimestamp


Interesting find. I have to use expect / actual sealed class because kotlin common and JS specific code are treated as different modules. this could be caused by the fact JS doesn't support packages.
unfortunately android studio check warns you to about exhaustive whens in this case

Thats weird...

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/TimestampSerializer.kt

firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/firestore.kt

avdyushin

Put some comments, overall looks good to me

vpodlesnyak added 2 commits May 13, 2022 09:30

fix encoding to correctly work on js + add serializers in tests becau…

671b064

…se of JS limitation

fix js tests

3317866

vpodlesnyak commented May 13, 2022

View reviewed changes

firebase-common/src/androidMain/kotlin/dev/gitlive/firebase/_encoders.kt Show resolved Hide resolved

vpodlesnyak commented May 13, 2022

View reviewed changes

re-enable git actions js tests

2600d88

vpodlesnyak force-pushed the fix-js-tests branch from a3fca6d to 2600d88 Compare May 13, 2022 07:54

vpodlesnyak added 5 commits May 13, 2022 12:28

update geo point implementation

23d1b1c

update document reference serialization

32915a4

create timestamp wrapper

800b99d

update timestamp and field value serialization

a731b46

test fixes and clenup

6ed6bb5

vpodlesnyak commented May 16, 2022

View reviewed changes

more cleanup + adding extra documentation

c945de1

vpodlesnyak force-pushed the fix-js-tests branch from 6e18846 to c945de1 Compare May 16, 2022 14:30

vpodlesnyak commented May 16, 2022

View reviewed changes

updated readme

4c09e93

vpodlesnyak marked this pull request as ready for review May 16, 2022 14:52

vpodlesnyak changed the title ~~Fix js tests~~ Fix js + enable js tests + code clean May 16, 2022

vpodlesnyak requested review from avdyushin and Daeda88 May 16, 2022 15:13

vpodlesnyak requested a review from thoutbeckers May 16, 2022 15:13

fixed android tests

d47223a

avdyushin reviewed May 17, 2022

View reviewed changes

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/GeoPointSerializer.kt Outdated Show resolved Hide resolved

avdyushin reviewed May 17, 2022

View reviewed changes

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/Timestamp.kt Outdated Show resolved Hide resolved

vpodlesnyak added 3 commits May 17, 2022 16:15

fix JS Timestamp equality

2c21e5b

update server Timestamp approach

9bef714

improve error handling in serializers

8c18801

vpodlesnyak commented May 18, 2022

View reviewed changes

vpodlesnyak requested review from avdyushin and arshadashrafi May 18, 2022 08:22