Adding Serialization fixes #448

Daeda88 · 2023-12-19T21:37:52Z

The current implementation of serializing classes does not work correctly when dealing with nested classes.

This PR aims to fix this by rewriting the encoding/decoding. This introduces EncodingSettings and DecodingSettings to Database, Firestore and Functions. On Firestore additional async support has been added

Daeda88 · 2023-12-19T21:48:26Z

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


 @Serializable
-data class TestData(val map: Map<String, String>, val bool: Boolean = false, val nullableBool: Boolean? = null)
+object TestObject {


Run these tests on the current project to see why encoding is currently broken

nbransby · 2023-12-29T23:43:44Z

are their any breaking changes in here?

…zation-fixes

Daeda88 · 2024-01-02T13:18:48Z

There are a few where encodeDefaults: Boolean was changed to encodeSettings: EncodeSettings. On some places I did add convenience extension methods, but that would require an import (and I also forgot in a few places). Ill try and add legacy accessors. This is gonna conflict with the where PR though, so it would be great if we can get that merged first so I can track the changes more easily.

Btw, if you're worried about API changes, consider adding https://github.com/Kotlin/binary-compatibility-validator to the project. We use it for Kaluga and it makes it much easier to spot changes

nbransby · 2024-01-02T20:39:56Z

Btw, if you're worried about API changes, consider adding https://github.com/Kotlin/binary-compatibility-validator to the project. We use it for Kaluga and it makes it much easier to spot changes

Yes we need this!

…zation-fixes

Daeda88 · 2024-01-03T16:15:33Z

Made changes to keep API stable

nbransby · 2024-01-04T10:45:23Z

The Java SDK has been updated to support latest firebase libs from android bom 32.7.0 and I have removed the jvmMain sources on master

Daeda88 · 2024-01-05T08:47:00Z

@nbransby seems publication to maven central failed: https://central.sonatype.com/artifact/dev.gitlive/firebase-java-sdk/versions

…zation-fixes

nbransby · 2024-01-10T14:43:27Z

@nbransby seems publication to maven central failed: https://central.sonatype.com/artifact/dev.gitlive/firebase-java-sdk/versions

fixed this now

nbransby · 2024-01-11T21:53:54Z

Found some more time to review...

I think we should keep named arguments instead of the EncodeSettings class, revert #449 (comment) and update the examples to some of the places the new arguments are added.

Having to wrap settings in EncodeSettings(...) is necessary boilerplate in the public API, people will likely only want to set 1 or 2 settings and named arguments with default values works great for that.

…zation-fixes

Daeda88 · 2024-01-12T16:35:30Z

Done

nbransby · 2024-01-12T18:54:09Z

firebase-common/src/commonMain/kotlin/dev/gitlive/firebase/EncodeDecodeSettings.kt

+sealed class EncodeDecodeSettings {
+
+    /**
+     * The [SerializersModule] to use for serialization. This allows for polymorphic serialization on runtime
+     */
+    abstract val serializersModule: SerializersModule
+}
+
+/**
+ * [EncodeDecodeSettings] used when encoding an object
+ * @property shouldEncodeElementDefault if `true` this will explicitly encode elements even if they are their default value
+ * @param serializersModule the [SerializersModule] to use for serialization. This allows for polymorphic serialization on runtime
+ */
+data class EncodeSettings(
+    val shouldEncodeElementDefault: Boolean = true,
+    override val serializersModule: SerializersModule = EmptySerializersModule(),
+) : EncodeDecodeSettings()
+
+/**
+ * [EncodeDecodeSettings] used when decoding an object
+ * @param serializersModule the [SerializersModule] to use for deserialization. This allows for polymorphic serialization on runtime
+ */
+data class DecodeSettings(
+    override val serializersModule: SerializersModule = EmptySerializersModule(),
+) : EncodeDecodeSettings()


cant these be internal now?

Some issues with some inline funs.

But actually, I'd like to propose that instead of named arguments we try a builder approach. E.g.:

set(value) { shouldEncodeDefaults = false }

Reason im suggesting it is because its annoying as hell right now to add new settings, since you have dozens of methods that need them specified. A builder makes it a one line addition

Uses builders instead of named arguments to improve maintainability

…zation-fixes

Daeda88 · 2024-01-12T20:37:10Z

Im trying a builder approach instead, as suggested here #448 (comment)

The advantage of this compared to named arguments is that it is much easier to maintain.

…zation-fixes

Daeda88 · 2024-01-19T12:57:00Z

@nbransby in case you werent aware, the builder approach is in the latest commit. Ive also tracked master again

nbransby · 2024-01-20T13:39:59Z

Should all functions that take a builder lambda be marked as inline to avoid the lambda allocation?

nbransby · 2024-01-20T13:55:30Z

Whats the point of the Async nested classes and the Deferred.convert function?

Daeda88 · 2024-01-22T11:06:47Z

Should all functions that take a builder lambda be marked as inline to avoid the lambda allocation?

I dont think that actually avoids anything. We can make the lambda nullable I guess

Whats the point of the Async nested classes and the Deferred.convert function?

Well, to improve async behaviour. For instance, logging or grouping. E.g.

suspend fun deleteSomeDocs() {
    val toExecute = listOf(
       firestore.collectionn("Foo").document("Bar").async.delete()
       firestore.collectionn("Foo").document("BarBar").async.delete()
    )
    println("Started deletion")
    // do some stuff in the mean time
    toExectute.awaitAll()
}

Deferred convert is a mapper for deferred. The Android sdk would return an AndroidDocumentReference and you'd want to return a Kotlin DocumentReference, so it should be mapped.

nbransby · 2024-01-22T11:40:29Z

I dont think that actually avoids anything. We can make the lambda nullable I guess

It does, thats the whole point of inline functions

nbransby · 2024-01-22T11:51:32Z

Well, to improve async behaviour. For instance, logging or grouping. E.g.

As this is just a generic async utility function and unrelated to firebase we shouldn't be introducing it to our public API, you are welcome to add it to the test sources if it makes writing tests easier

nbransby · 2024-01-22T11:57:21Z

Deferred convert is a mapper for deferred. The Android sdk would return an AndroidDocumentReference and you'd want to return a Kotlin DocumentReference, so it should be mapped.

Same with this, move it to test sources if you use it in the tests

More in line with old code and probably more stable since we're dealing with maps

nbransby · 2024-02-07T09:18:18Z

Looking good, whats the purpose of the Base... and Native... classes such as NativeQuery and s BaseTransaction? @Daeda88

Daeda88 · 2024-02-07T10:42:06Z

Looking good, whats the purpose of the Base... and Native... classes such as NativeQuery and s BaseTransaction? @Daeda88

So with expect/actuals classes you cannot implement code on the common side. Even though with a lot of the code, you kind of want it (e.g., most methods need to encode/decode their values). Base solves this by introducing an abstract class that the expect/actuals classes implement. It makes code much more maintainable and also testable since they can be mocked. Native is a solution to allow for common constructors. This is useful for the abstract classes (e.g. BaseDatabaseReference) that may map the common behaviour. All in all, its to reduce code duplication.

nbransby · 2024-02-16T06:37:45Z

Even though with a lot of the code, you kind of want it (e.g., most methods need to encode/decode their values). Base solves this by introducing an abstract class that the expect/actuals classes implement.

I assume this is to share code internally within the library? I also assume we don't need to share additional internal state (the only state our wrapper classes should ever hold is the reference to the native instance¹ in which case inheritance is probably not the right tool here, wouldn't internal extension functions be a better fit? Want to prevent polluting the public API with inheritance chains when their only purpose is internal code sharing.

in fact one day it would be nice if our wrapper classes were compiled away at build time) ↩

Daeda88 · 2024-02-16T12:56:34Z

Even though with a lot of the code, you kind of want it (e.g., most methods need to encode/decode their values). Base solves this by introducing an abstract class that the expect/actuals classes implement.

I assume this is to share code internally within the library? I also assume we don't need to share additional internal state (the only state our wrapper classes should ever hold is the reference to the native instance1 in which case inheritance is probably not the right tool here, wouldn't internal extension functions be a better fit? Want to prevent polluting the public API with inheritance chains when their only purpose is internal code sharing.

Footnotes
1. in fact one day it would be nice if our wrapper classes were [compiled away at build time](https://github.com/GitLiveApp/firebase-kotlin-sdk/issues/132)) [↩](#user-content-fnref-1-6c88b1acbb299166cc14a4d10e5c5c3e)

There's no state being stored whatsoever. This is about common code having common behaviour. To give an example with the changes:

Old:

// Common
expect class DocumentReference internal constructor(nativeValue: NativeDocumentReference) {
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean = true, merge: Boolean = false)
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean = true, vararg mergeFields: String)
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean = true, vararg mergeFieldPaths: FieldPath)

    suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean = true, merge: Boolean = false)
    suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean = true, vararg mergeFields: String)
    suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean = true, vararg mergeFieldPaths: FieldPath)
}

// Android
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) {
    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, merge: Boolean) = when(merge) {
        true -> android.set(encode(data, encodeDefaults)!!, SetOptions.merge())
        false -> android.set(encode(data, encodeDefaults)!!)
    }.await().run { Unit }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        android.set(encode(data, encodeDefaults)!!, SetOptions.mergeFields(*mergeFields))
            .await().run { Unit }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        android.set(encode(data, encodeDefaults)!!, SetOptions.mergeFieldPaths(mergeFieldPaths.map { it.android }))
            .await().run { Unit }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, merge: Boolean) = when(merge) {
        true -> android.set(encode(strategy, data, encodeDefaults)!!, SetOptions.merge())
        false -> android.set(encode(strategy, data, encodeDefaults)!!)
    }.await().run { Unit }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        android.set(encode(strategy, data, encodeDefaults)!!, SetOptions.mergeFields(*mergeFields))
            .await().run { Unit }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        android.set(encode(strategy, data, encodeDefaults)!!, SetOptions.mergeFieldPaths(mergeFieldPaths.map { it.android }))
            .await().run { Unit }
}

// iOS
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) {
    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, merge: Boolean) =
        await { ios.setData(encode(data, encodeDefaults)!! as Map<Any?, *>, merge, it) }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        await { ios.setData(encode(data, encodeDefaults)!! as Map<Any?, *>, mergeFields.asList(), it) }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        await { ios.setData(encode(data, encodeDefaults)!! as Map<Any?, *>, mergeFieldPaths.map { it.ios }, it) }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, merge: Boolean) =
        await { ios.setData(encode(strategy, data, encodeDefaults)!! as Map<Any?, *>, merge, it) }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        await { ios.setData(encode(strategy, data, encodeDefaults)!! as Map<Any?, *>, mergeFields.asList(), it) }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        await { ios.setData(encode(strategy, data, encodeDefaults)!! as Map<Any?, *>, mergeFieldPaths.map { it.ios }, it) }
}

// JS
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) {
    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, merge: Boolean) =
        rethrow { setDoc(js, encode(data, encodeDefaults)!!, json("merge" to merge)).await() }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        rethrow { setDoc(js, encode(data, encodeDefaults)!!, json("mergeFields" to mergeFields)).await() }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        rethrow { setDoc(js, encode(data, encodeDefaults)!!, json("mergeFields" to mergeFieldPaths.map { it.js }.toTypedArray())).await() }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, merge: Boolean) =
        rethrow { setDoc(js, encode(strategy, data, encodeDefaults)!!, json("merge" to merge)).await() }
    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        rethrow { setDoc(js, encode(strategy, data, encodeDefaults)!!, json("mergeFields" to mergeFields)).await() }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        rethrow { setDoc(js, encode(strategy, data, encodeDefaults)!!, json("mergeFields" to mergeFieldPaths.map { it.js }.toTypedArray())).await() }
}

All these expect/actuals funs can and should be split up into two parts:

A common behaviour, the encoding.
A platform specific behaviour, here set data

Using abstract classes to do part 1 in common code, and then forwarding the result to 2 to do something expect/actuals reduces the footprint greatly:

// Common
abstract class BaseDocumentReference {
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, merge: Boolean = false) = setEncoded(encode(data, encodeDefaults)!!, if (merge) SetOptions.Merge else SetOptions.Overwrite)
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFields: String) = setEncoded(encode(data, encodeDefaults)!!, SetOptions.MergeFields(mergeFields.asList()))
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) = setEncoded(encode(data, encodeDefaults)!!, SetOptions.MergeFieldPaths(mergeFieldPaths.asList()))

suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, merge: Boolean = false) = setEncoded(
        encode(strategy, data, encodeDefaults)!!, if (merge) SetOptions.Merge else SetOptions.Overwrite)
suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFields: String) = setEncoded(
        encode(strategy, data, encodeDefaults)!!, SetOptions.MergeFields(mergeFields.asList()))
suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) = setEncoded(
        encode(strategy, data, encodeDefaults)!!, SetOptions.MergeFieldPaths(mergeFieldPaths.asList()))

    @PublishedApi
    internal abstract suspend fun setEncoded(encodedData: Any, setOptions: SetOptions)
}

expect class DocumentReference internal constructor(nativeValue: NativeDocumentReference) : BaseDocumentReference

// Android

val SetOptions.android: com.google.firebase.firestore.SetOptions? get() = when (this) {
    is SetOptions.Merge -> com.google.firebase.firestore.SetOptions.merge()
    is SetOptions.Overwrite -> null
    is SetOptions.MergeFields -> com.google.firebase.firestore.SetOptions.mergeFields(fields)
    is SetOptions.MergeFieldPaths -> com.google.firebase.firestore.SetOptions.mergeFieldPaths(encodedFieldPaths)
}
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) : BaseDocumentReference() {
    override suspend fun setEncoded(encodedData: Any, setOptions: SetOptions) {
        val task = (setOptions.android?.let {
            android.set(encodedData, it)
        } ?: android.set(encodedData))
        task.await()
    }
}

// iOS
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) : BaseDocumentReference() {
    override suspend fun setEncoded(encodedData: Any, setOptions: SetOptions) = await {
        when (setOptions) {
            is SetOptions.Merge -> ios.setData(encodedData as Map<Any?, *>, true, it)
            is SetOptions.Overwrite -> ios.setData(encodedData as Map<Any?, *>, false, it)
            is SetOptions.MergeFields -> ios.setData(encodedData as Map<Any?, *>, setOptions.fields, it)
            is SetOptions.MergeFieldPaths -> ios.setData(encodedData as Map<Any?, *>, setOptions.encodedFieldPaths, it)
        }
    }
}

// JS
val SetOptions.js: Json get() = when (this) {
    is SetOptions.Merge -> json("merge" to true)
    is SetOptions.Overwrite -> json("merge" to false)
    is SetOptions.MergeFields -> json("mergeFields" to fields.toTypedArray())
    is SetOptions.MergeFieldPaths -> json("mergeFields" to encodedFieldPaths.toTypedArray())
}

actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) : BaseDocumentReference() {
    override suspend fun setEncoded(encodedData: Any, setOptions: SetOptions) = rethrow {
        setDoc(js, encodedData, setOptions.js).await()
    }
}

Consider how much more maintainable this is. We encode consistently on a single place, and we send to the platform logic in a single place. Before, for these 6 methods, we encoded for each method per platform (so 3 times 6 = 18 times) and for each method we had to write the same coroutine logic with error handling (also 18). With using abstract classes we get 6 encodings and 3 platform methods.

You can't do this with extension functions, because that would defeat the purpose where the common step still needs to be implemented on each method per platform.

Of course, instead of abstract classes this could be done by interface delegation, but at that point, we will have added extra state to the classes. Similarly im not too fond of using default implementations to the interfaces here, as it would basically constitute an abstract class.

nbransby · 2024-02-16T13:09:38Z

You can't do this with extension functions, because that would defeat the purpose where the common step still needs to be implemented on each method per platform.

Are you saying this pattern is not possible?

common

expect class Something {
  expect fun publicApi()
}

internal fun Something.sharedFunction() {}

platform

actual class Something() {
  actual fun publicApi() {
    sharedFunction()
    //do other platform specific stuff
  }
}

Daeda88 · 2024-02-16T13:17:25Z

No, im saying its not correct to do it like that in my opinion, for maintainability reasons. Because if you use that approach, every time something changes to sharedFunction you have to account for it in 3 times as many places.

Daeda88 · 2024-02-16T13:32:23Z

At the very least, we should have
common

expect class Something {
  expect fun publicApi(parameter: String): Result
}

internal inline fun Something.sharedFunction(parameter: String, parsedMethod: (Any) -> Result): Result

platform

actual class Something() {
  actual fun publicApi(parameter: String) = sharedFunction(parameter) { any ->
     any.toResult()
  } 
}

But I dont think that solves the maintainability problem per se. You'll still have to write 3 times as many implementations.

Daeda88 · 2024-02-16T14:03:13Z

If you want to account for value classes, I would propose to move to interfaces with default implementations, as that sort of meets both criteria (maintainability and maintaining possibility to inline)
https://play.kotlinlang.org/#eyJ2ZXJzaW9uIjoiMS45LjIyIiwicGxhdGZvcm0iOiJqYXZhIiwiYXJncyI6IiIsIm5vbmVNYXJrZXJzIjp0cnVlLCJ0aGVtZSI6ImlkZWEiLCJjb2RlIjoiLyoqXG4gKiBZb3UgY2FuIGVkaXQsIHJ1biwgYW5kIHNoYXJlIHRoaXMgY29kZS5cbiAqIHBsYXkua290bGlubGFuZy5vcmdcbiAqL1xuXG5pbXBvcnQga290bGlueC5jb3JvdXRpbmVzLipcblxuaW50ZXJmYWNlIFRlc3Qge1xuICAgIHZhbCBzdHJpbmc6IFN0cmluZ1xuICAgIGZ1biB0ZXN0RnVuKCk6IFN0cmluZyB7IHJldHVybiBcInRlc3Qkc3RyaW5nXCIgfVxufVxuXG5ASnZtSW5saW5lXG52YWx1ZSBjbGFzcyBUZXN0Q2xhc3Mob3ZlcnJpZGUgdmFsIHN0cmluZzogU3RyaW5nKSA6IFRlc3RcblxuZnVuIG1haW4oKSB7XG4gICAgcHJpbnRsbihUZXN0Q2xhc3MoXCJIZWxsb1dvcmxkXCIpLnRlc3RGdW4oKSlcbn0ifQ==

nbransby · 2024-02-16T14:35:45Z

No, im saying its not correct to do it like that in my opinion, for maintainability reasons. Because if you use that approach, every time something changes to sharedFunction you have to account for it in 3 times as many places.

I'm a little confused here as I don't see how inheritance would make it any better, for reference, here's the inheritance version of my trivial example, as you can see the actual implementation doesn't change but we end up publishing BaseToShareStuffBetweenTheActualImplementations in our public API.

common

expect class Something : BaseToShareStuffBetweenTheActualImplementations {
  expect fun publicApi()
}

abstract class BaseToShareStuffBetweenTheActualImplementations {
  internal fun sharedFunction() {}
}

platform

actual class Something() {
  actual fun publicApi() {
    sharedFunction()
    //do other platform specific stuff
  }
}

nbransby · 2024-02-16T14:38:10Z

If you want to account for value classes, I would propose to move to interfaces with default implementations, as that sort of meets both criteria (maintainability and maintaining possibility to inline)

Definitely an option but we should hold fire on a refactor like this until the expect/actual language feature is stabilized

Daeda88 · 2024-02-16T15:41:09Z

You example is the reverse of what we need though.
It would be:

common

expect class Something : BaseToShareStuffBetweenTheActualImplementations()

abstract class BaseToShareStuffBetweenTheActualImplementations {
  fun publicApi() {
          doSomethingCommon()
          platformFunction()
    }
  internal abstract fun platformFunction()
}

platform

actual class Something() : BaseToShareStuffBetweenTheActualImplementations() {
  override fun platformFunction() {
    //do other platform specific stuff
  }
}

There's a big difference. The abstract class serves only to expose the public API methods and handle default implementation for it. It then splits up the public API method in something that can be done in common and something that must be done on the platform.

The public API of Something here exposes only publicAPI(). The difference between only and new logic is only that the publicAPI() method is now not an expect/actuals method but one inherited from BaseToShareStuffBetweenTheActualImplementations

Daeda88 · 2024-02-16T16:04:31Z

To compare:

// Common
expect class TestClass {
    fun publicAPI()
}

internal fun TestClass.sharedFunction() {}

// Platform
actual class TestClass {
    actual fun publicAPI() {
        sharedFunction()
    }
}

Gives

public final class TestClass {
	public fun <init> ()V
	public final fun publicAPI ()V
}

Whereas

// Common
abstract class BaseTestClass {
    fun publicApi() {
        platformFunction()
    }
    internal abstract fun platformFunction()
}

expect class TestClass : BaseTestClass

// Platform
actual class TestClass : BaseTestClass() {
    override fun platformFunction() {}
}

Gives us

public abstract class BaseTestClass {
	public fun <init> ()V
	public final fun publicApi ()V
}

public final class TestClass : BaseTestClass {
	public fun <init> ()V
}

While its true this exposes the BaseTestClass as well, you shoud keep in mind that we dont need to do

actual fun publicApi() { 
   sharedFunction()
    //do other platform specific stuff
}

in the code for every single platform. That is the benefit your get from this. I've had to make changes to nearly all functions related to encoding for these changes. Thats literally dozens if not hundreds of methods. An abstract class pattern reduces the number of places where you need to apply changes by 66%.

nbransby · 2024-02-17T01:00:57Z

There's a big difference. The abstract class serves only to expose the public API methods and handle default implementation for it. It then splits up the public API method in something that can be done in common and something that must be done on the platform.

Ok I understand now! So in that case you could just do:

common

class Something {
  fun publicApi() {
        doSomethingCommon()
        platformFunction()
    }
}

expect internal fun Something.platformFunction()

platform

actual fun Something.platformFunction() {
    //do other platform specific stuff
}

No inheritance needed. But I guess this causes an issue due to not having the native instance property (ios, android, js) in the class

nbransby · 2024-02-17T01:48:22Z

But I guess this causes an issue due to not having the native instance property (ios, android, js) in the class

Which really can only be solved with what you have implemented. One proposal though, since the base classes contain the public API why don't we call those the original names (eg DatabaseReference)and then make the platform specific subclasses internal (eg JsDatabaseReference)?

There is still the issue of the publicly accessible android, ios, and js properties but they could be implemented as follows:

database.js.kt

val DatabaseReference.js get() = (this as JsDatabaseReference).js

Daeda88 · 2024-02-17T19:44:11Z

But I guess this causes an issue due to not having the native instance property (ios, android, js) in the class

Which really can only be solved with what you have implemented. One proposal though, since the base classes contain the public API why don't we call those the original names (eg DatabaseReference)and then make the platform specific subclasses internal (eg JsDatabaseReference)?

There is still the issue of the publicly accessible android, ios, and js properties but they could be implemented as follows:

database.js.kt
val DatabaseReference.js get() = (this as JsDatabaseReference).js

Glad you get my considerations. Im happy to do the renames, but keep in mind that this makes the upgrade less frictionless since people would have to import the js extension method. If thats fine by you, ill make the renames somewhere in the coming week

nbransby · 2024-02-17T23:20:09Z

but keep in mind that this makes the upgrade less frictionless since people would have to import the js extension method

true, means its a breaking change and we'll need to bump the major version number and change the js, android and `ios properties on all classes to extensions. The classes that haven't had a base class introduced could do this:

actual class Database(internal val android: com.firebase.Database)

val Database.android get() = android

…zation-fixes

Daeda88 · 2024-03-01T16:14:46Z

@nbransby finally got around to looking at this. I removed abstract classes in favour of expected wrappers.

so instead of

abstract class BaseSomething {
   fun doSomething() {
       doCommon()
       doPlatformSpecific()
   }
   internal abstract fun doPlatformSpecific()
}

expect class Something : BaseSomething {
   fun commonPublicAPIButPlatformSpecificImplementation()
}

I now have

internal expect class NativeSomething {
   fun doPlatformSpecific()
   fun commonPublicAPIButPlatformSpecificImplementation()
}

class Something internal constructor(internal val native: NativeSomething) {
  fun doSomething() {
       doCommon()
       native.doPlatformSpecific()
   }
   
   fun commonPublicAPIButPlatformSpecificImplementation() = native.commonPublicAPIButPlatformSpecificImplementation()
}

nbransby · 2024-04-09T07:32:49Z

@Daeda88 I believe this might of introduced a bug in the rtdb on js - passing a map to updateChildren with null values would delete those keys from the rtdb if they exist, this still appears to work on android/jvm but for js it causes permission denied with a rule such as this:

          "lastActivity": { ".validate": "!newData.exists() || newData.isNumber()" }

My guess is that the serialization changes have caused null instead of undefined to be passed to the firebase-js-sdk causing it to attempt to set the value to null instead of removing the key.

Adding Serialization fixes

816bd62

Daeda88 commented Dec 19, 2023

View reviewed changes

Merge remote-tracking branch 'GitLiveApp/master' into feature/seriali…

e126519

…zation-fixes

Daeda88 added 3 commits January 3, 2024 15:38

Ensure API stability

69e4516

Merge remote-tracking branch 'GitLiveApp/master' into feature/seriali…

582532d

…zation-fixes

Track for JS

fae5299

Merge remote-tracking branch 'GitLiveApp/master' into feature/seriali…

0d252b5

…zation-fixes

Daeda88 added 2 commits January 12, 2024 17:34

Use named arguments for settings

a4a7150

Merge remote-tracking branch 'GitLiveApp/master' into feature/seriali…

0c6e22c

…zation-fixes

nbransby reviewed Jan 12, 2024

View reviewed changes

Daeda88 added 2 commits January 12, 2024 21:36

Use builders instead

8305507

Uses builders instead of named arguments to improve maintainability

Merge remote-tracking branch 'GitLiveApp/master' into feature/seriali…

045e127

…zation-fixes

Merge remote-tracking branch 'GitLiveApp/master' into feature/seriali…

3c9f7ff

…zation-fixes

Daeda88 added 3 commits January 29, 2024 11:38

Small test improvement

1995071

Decode polymorphic using elementName rather than index

87d4ae2

More in line with old code and probably more stable since we're dealing with maps

Also test nested data class encoding

71bcce4

Daeda88 added 3 commits March 1, 2024 14:15

Merge remote-tracking branch 'GitLiveApp/master' into feature/seriali…

26d7ea9

…zation-fixes

Use Wrappers rather than Abstract classes

f75b240

Use wrappes + extension methods instead of abstract class

f8eb4fd

nbransby self-requested a review March 29, 2024 12:41

nbransby approved these changes Mar 29, 2024

View reviewed changes

nbransby merged commit 71bcb81 into GitLiveApp:master Mar 29, 2024
3 checks passed

This was referenced Apr 10, 2024

Map null in map to undefined for JS database #493

Closed

Fix OnDisconnect failing to updateChildren #494

Merged

Add Asynchronouts/Deferred implementation to Firestore/Database/etc #497

Open

Adding Serialization fixes #448

Adding Serialization fixes #448

Conversation

Daeda88 commented Dec 19, 2023

Daeda88 Dec 19, 2023

Choose a reason for hiding this comment

nbransby commented Dec 29, 2023

Daeda88 commented Jan 2, 2024

nbransby commented Jan 2, 2024

Daeda88 commented Jan 3, 2024

nbransby commented Jan 4, 2024

Daeda88 commented Jan 5, 2024

nbransby commented Jan 10, 2024

nbransby commented Jan 11, 2024

Daeda88 commented Jan 12, 2024

nbransby Jan 12, 2024

Choose a reason for hiding this comment

Daeda88 Jan 12, 2024

Choose a reason for hiding this comment

Daeda88 commented Jan 12, 2024

Daeda88 commented Jan 19, 2024

nbransby commented Jan 20, 2024

nbransby commented Jan 20, 2024

Daeda88 commented Jan 22, 2024 • edited Loading

nbransby commented Jan 22, 2024

nbransby commented Jan 22, 2024

nbransby commented Jan 22, 2024

nbransby commented Feb 7, 2024

Daeda88 commented Feb 7, 2024

nbransby commented Feb 16, 2024

Footnotes

Daeda88 commented Feb 16, 2024 • edited Loading

Footnotes

nbransby commented Feb 16, 2024 • edited Loading

Daeda88 commented Feb 16, 2024

Daeda88 commented Feb 16, 2024 • edited Loading

Daeda88 commented Feb 16, 2024

nbransby commented Feb 16, 2024

nbransby commented Feb 16, 2024 • edited Loading

Daeda88 commented Feb 16, 2024 • edited Loading

Daeda88 commented Feb 16, 2024

nbransby commented Feb 17, 2024 • edited Loading

nbransby commented Feb 17, 2024

Daeda88 commented Feb 17, 2024

nbransby commented Feb 17, 2024

Daeda88 commented Mar 1, 2024 • edited Loading

nbransby commented Apr 9, 2024

Daeda88 commented Jan 22, 2024 •

edited

Loading

Daeda88 commented Feb 16, 2024 •

edited

Loading

nbransby commented Feb 16, 2024 •

edited

Loading

Daeda88 commented Feb 16, 2024 •

edited

Loading

nbransby commented Feb 16, 2024 •

edited

Loading

Daeda88 commented Feb 16, 2024 •

edited

Loading

nbransby commented Feb 17, 2024 •

edited

Loading

Daeda88 commented Mar 1, 2024 •

edited

Loading