Added Settings to deal with newer settings & add callback threads #450

Daeda88 · 2023-12-20T16:34:07Z

Final one for now:

This one updates Firestore/Database to provide settings. This includes fixes for deprecated Persistency settings (though not on Java as that is not tracked yet) and adds support on iOS/Android to set custom callback dispatchers. Otherwise, Firebase internally still goes to the main thread even when running in a background thread.

…re-settings

Daeda88 · 2024-01-03T16:16:31Z

@nbransby this is now tracking master, which means the JVM split for firestore is tracked. This PR can now be properly reviewed

…re-settings

Daeda88 · 2024-04-03T10:23:46Z

Updated tracking again @nbransby

…re-settings

nbransby · 2024-04-13T05:20:13Z

firebase-database/src/commonMain/kotlin/dev/gitlive/firebase/database/database.kt

    fun reference(path: String): DatabaseReference
    fun reference(): DatabaseReference
-    fun setPersistenceEnabled(enabled: Boolean)
+    fun setSettings(settings: Settings)


why not just
fun setSettings(persistenceEnabled: Boolean = false, persistenceCacheSizeBytes: Long? = null)
What the point of making everyone write the boilerplate
database.setSettings(FirebaseDatabase.Settings.createSettings(persistenceEnabled = true, persistenceCacheSizeBytes = 2048))
?

nbransby · 2024-04-13T05:25:38Z

Looking at the official android sdk, the methods are defined as follows:
https://firebase.google.com/docs/reference/android/com/google/firebase/database/FirebaseDatabase#setPersistenceCacheSizeBytes(long)
Any reason not to match this?

nbransby · 2024-04-13T06:35:03Z

This PR inspired me to write a section on compatibility with the official firebase android sdk here: https://github.com/GitLiveApp/firebase-kotlin-sdk?tab=readme-ov-file#compatibility-with-the-official-firebase-android-sdk

Can we apply those points to this PR?

Daeda88 · 2024-04-14T17:20:14Z

Yep, will track/correct when I have the time

…re-settings

Daeda88 · 2024-04-15T10:49:50Z

Changes now track the Android API. I did leave the LocalCacheSettings as a sealed class as opposed to an interface, so it can both be constructed through a direct constructor as well as through builders as they exist on Android.

nbransby · 2024-04-17T12:00:36Z

firebase-database/src/iosMain/kotlin/dev/gitlive/firebase/database/database.kt

+    fun setCallbackQueue(callbackQueue: dispatch_queue_t) {
+        ios.callbackQueue = callbackQueue
+    }


lets not start adding coverage for platform specific APIs, let users use .ios

nbransby · 2024-04-17T12:08:19Z

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

+@Deprecated("Use dev.gitlive.firebase.firestore instead", replaceWith = ReplaceWith("setSettings(FirebaseFirestore.Settings.create())"))
+fun FirebaseFirestore.setSettings(
+    persistenceEnabled: Boolean? = null,
+    sslEnabled: Boolean? = null,
+    host: String? = null,
+    cacheSizeBytes: Long? = null,
+) {
+    settings = firestoreSettings {
+        this.sslEnabled = sslEnabled ?: true
+        this.host = host ?: FirestoreSettings.DEFAULT_HOST
+        this.cacheSettings = if (persistenceEnabled != false) {
+            LocalCacheSettings.Persistent(cacheSizeBytes ?: FirestoreSettings.CACHE_SIZE_UNLIMITED)
+        } else {
+            val cacheSize = cacheSizeBytes ?: FirestoreSettings.CACHE_SIZE_UNLIMITED
+            val garbageCollectionSettings = if (cacheSize == FirestoreSettings.CACHE_SIZE_UNLIMITED) {
+                GarbageCollectorSettings.Eager
+            } else {
+                GarbageCollectorSettings.LRUGC(cacheSize)
+            }
+            LocalCacheSettings.Memory(garbageCollectionSettings)
+        }
+    }
+}
+
 @PublishedApi


moving this to an extension will make it a breaking change, why not leave it in the class?

FirebaseFirestore is expect/actuals so it cant have default implementation. Omving to the same NativeWrapper pattern used elsewhere now

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

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

nbransby

I didn't comment on every change needed to comply with https://github.com/GitLiveApp/firebase-kotlin-sdk?tab=readme-ov-file#compatibility-with-the-official-firebase-android-sdk but there should be enough comments there so you can apply the same changes to the remaining classes, fields and functions

nbransby · 2024-04-17T12:17:14Z

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

+val FirestoreSettings.Companion.CACHE_SIZE_UNLIMITED get() = -1L
+val FirestoreSettings.Companion.DEFAULT_HOST get() = "firestore.googleapis.com"
+
+expect class FirestoreSettings {


rename FirebaseFirestoreSettings to match android

nbransby · 2024-04-17T12:18:12Z

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

    suspend fun disableNetwork()
    suspend fun enableNetwork()
 }

+val FirestoreSettings.Companion.CACHE_SIZE_UNLIMITED get() = -1L


move inside companion object for binary compat with android

nbransby · 2024-04-17T12:18:33Z

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

    suspend fun disableNetwork()
    suspend fun enableNetwork()
 }

+val FirestoreSettings.Companion.CACHE_SIZE_UNLIMITED get() = -1L
+val FirestoreSettings.Companion.DEFAULT_HOST get() = "firestore.googleapis.com"


I dont see this property on android?

https://github.com/firebase/firebase-android-sdk/blob/1d717ea6c7f1feb3219b254008212e8e73b9ce18/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestoreSettings.java#L33

https://stackoverflow.com/questions/17035271/what-does-hide-mean-in-the-android-source-code

Ill make it internal only

nbransby · 2024-04-17T12:26:09Z

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

+
+    companion object {}
+
+    interface Builder {


make this a class with the constructors available here to match android:
https://firebase.google.com/docs/reference/android/com/google/firebase/firestore/FirebaseFirestoreSettings.Builder

nbransby · 2024-04-17T12:30:49Z

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

@@ -0,0 +1,90 @@
+package dev.gitlive.firebase.firestore
+
+sealed class LocalCacheSettings {


make sealed interface to match android

nbransby · 2024-04-17T12:31:26Z

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

+    internal companion object {
+        // Firestore cache defaults to 100MB
+        const val DEFAULT_CACHE_SIZE = 100L*1024L*1024L
+    }


dont see this in the android sdk?

https://github.com/firebase/firebase-android-sdk/blob/1d717ea6c7f1feb3219b254008212e8e73b9ce18/firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestoreSettings.java#L36

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

nbransby · 2024-04-17T12:35:22Z

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

+            override fun build(): Persistent = Persistent(sizeBytes)
+        }
+    }
+    data class Memory(val garbaseCollectorSettings: GarbageCollectorSettings) : LocalCacheSettings() {


make constructor internal or private

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

nbransby · 2024-04-17T12:41:33Z

Also I used the term binary compat a lot when technically I mean source compatibility

Daeda88 · 2024-04-17T19:29:40Z

Updated with fixes for remarks. Also moved the IllegalStateException thrown when attempting to set new settings after using Firestore into the common code, so that it may be caught properly (before it was impossible to do so on iOS)

Daeda88 · 2024-04-17T19:31:02Z

BTW: we have the extent methods
val FirebaseFirestore.android etc now, but I think I found a way to bring it back into the class itself. Since it's more relevant to the other PR however, Ill add it there instead

nbransby · 2024-04-18T04:15:50Z

Also moved the IllegalStateException thrown when attempting to set new settings after using Firestore into the common code, so that it may be caught properly (before it was impossible to do so on iOS)

Let's not start copying business logic such as this from the native SDKs as its causes a maintenance headache - you can see here the logic is more complex. Lets leverage our dependency on the native SDKs and let them do the validation.

The fact you can't catch it on iOS is a good thing, an assertion like this represents a programming error, not expected runtime behavior.

Daeda88 · 2024-04-18T06:21:13Z

Removed then

nbransby · 2024-04-18T16:43:36Z

look like the build failed:
e: file:///home/runner/work/firebase-kotlin-sdk/firebase-kotlin-sdk/firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt:47:17 Unresolved reference: isConfigured

…re-settings

Daeda88 · 2024-04-18T18:27:23Z

Whoops, dont do things quickly in the morning :')

nbransby · 2024-04-19T01:00:55Z

Js test still failing 😅

Daeda88 · 2024-04-19T07:51:55Z

Source changes pointed to an actual bug. Fixed now

Daeda88 added 2 commits December 20, 2023 17:25

Added Settings to deal with newer settings & add callback threads

c80c5f2

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

2e9cc91

…re-settings

Daeda88 added 2 commits January 5, 2024 09:48

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

fdba4bf

…re-settings

Merge branch 'master' into feature/firestore-settings

c76fba7

nbransby and others added 2 commits April 3, 2024 23:42

Merge branch 'master' into feature/firestore-settings

d144ea2

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

56a17e2

…re-settings

nbransby reviewed Apr 13, 2024

View reviewed changes

Daeda88 added 2 commits April 14, 2024 22:57

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

f063b30

…re-settings

Use API aligned with Android specifications

93d88f4

Daeda88 requested a review from nbransby April 15, 2024 10:52

Daeda88 added 2 commits April 15, 2024 12:59

Make iOS emulator track settings correctly

473e9ba

Minor cleanup/platform consistency fixes

d7b3719

Daeda88 mentioned this pull request Apr 17, 2024

Fix OnDisconnect failing to updateChildren #494

Merged

nbransby reviewed Apr 17, 2024

View reviewed changes

firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/firestore.kt Show resolved Hide resolved

nbransby reviewed Apr 17, 2024

View reviewed changes

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

nbransby requested changes Apr 17, 2024

View reviewed changes

PR remarks

96df881

Daeda88 requested a review from nbransby April 17, 2024 19:28

Removed IllegalStateException

f6b41ca

Daeda88 added 2 commits April 18, 2024 20:24

Fixed missing revert

a535e95

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

844f0b4

…re-settings

Fixed failing test

5a28653

nbransby approved these changes Apr 20, 2024

View reviewed changes

nbransby merged commit 8daf907 into GitLiveApp:master Apr 20, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Settings to deal with newer settings & add callback threads #450

Added Settings to deal with newer settings & add callback threads #450

Daeda88 commented Dec 20, 2023 •

edited

Loading

Daeda88 commented Jan 3, 2024

Daeda88 commented Apr 3, 2024

nbransby Apr 13, 2024

nbransby commented Apr 13, 2024

nbransby commented Apr 13, 2024 •

edited

Loading

Daeda88 commented Apr 14, 2024

Daeda88 commented Apr 15, 2024

nbransby Apr 17, 2024

nbransby Apr 17, 2024

Daeda88 Apr 17, 2024

nbransby left a comment

nbransby Apr 17, 2024

nbransby Apr 17, 2024

nbransby Apr 17, 2024

Daeda88 Apr 17, 2024

nbransby Apr 17, 2024

Daeda88 Apr 17, 2024

nbransby Apr 17, 2024

nbransby Apr 17, 2024 •

edited

Loading

nbransby Apr 17, 2024

Daeda88 Apr 17, 2024

nbransby Apr 17, 2024

nbransby commented Apr 17, 2024

Daeda88 commented Apr 17, 2024

Daeda88 commented Apr 17, 2024 •

edited

Loading

nbransby commented Apr 18, 2024

Daeda88 commented Apr 18, 2024

nbransby commented Apr 18, 2024

Daeda88 commented Apr 18, 2024

nbransby commented Apr 19, 2024

Daeda88 commented Apr 19, 2024

		@@ -0,0 +1,90 @@
		package dev.gitlive.firebase.firestore

		sealed class LocalCacheSettings {

Added Settings to deal with newer settings & add callback threads #450

Added Settings to deal with newer settings & add callback threads #450

Conversation

Daeda88 commented Dec 20, 2023 • edited Loading

Daeda88 commented Jan 3, 2024

Daeda88 commented Apr 3, 2024

Choose a reason for hiding this comment

nbransby commented Apr 13, 2024

nbransby commented Apr 13, 2024 • edited Loading

Daeda88 commented Apr 14, 2024

Daeda88 commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbransby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbransby Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbransby commented Apr 17, 2024

Daeda88 commented Apr 17, 2024

Daeda88 commented Apr 17, 2024 • edited Loading

nbransby commented Apr 18, 2024

Daeda88 commented Apr 18, 2024

nbransby commented Apr 18, 2024

Daeda88 commented Apr 18, 2024

nbransby commented Apr 19, 2024

Daeda88 commented Apr 19, 2024

Daeda88 commented Dec 20, 2023 •

edited

Loading

nbransby commented Apr 13, 2024 •

edited

Loading

nbransby Apr 17, 2024 •

edited

Loading

Daeda88 commented Apr 17, 2024 •

edited

Loading