Fixed a crash for settings when iOS would access the same Firebase instance twice

[Daeda88]

Fixes #552

[nbransby]

Why does accessing the same Firebase instance twice crash on iOS?

[Daeda88]

iOS natively crashes when you change settings after doing something with firstore (e.g. disable the network). But the code contained this to sync settings:

    actual var settings: FirebaseFirestoreSettings = firestoreSettings { }.also {
        native.settings = it.ios
    }

So native settings would be set to firestoreSettings { } whenever a Kotlin FirebaseFirestore instance was created.
So if you do:

Firebase.firestore.disableNetwork()
Firebase.firestore.enableNetwork()

What happens is:

• Grab default firestore app
• Set its settings to the default
• Disable network
• Grab default firestore app
• Set its settings to the default <-- This is not allowed

Something similar is happening on JS as per #551, Ill try and create a fix for that one too

[Daeda88]

Latest commit adds a test & fixes #551

[nbransby]

I thought we removed this check in favour of letting the native sdk throw the exception?

[Daeda88]

The thing is that there is no native sdk to fall back to, as explained in the other remark. We shouldnt be calling initializeFirestore here yet, as settings may still be updated after this.

[nbransby]

what are we doing exactly that is straightforward on android but not supported natively on ios and js? The other option here would be to cater for the lowest common denominator, in other words don't support it on android either, but I'm still not exactly clear what the differences are?

[Daeda88]

So there are two things:

First and foremost, the biggest difference between the mobile platforms and JS is that for the mobile platforms, the FirebaseFirestore object internally keeps a state machine that allows it to change settings until it is first used. So, you can do:

firestore.settings.host = "1.1.1.1"
firestore.enableNetwork()
firstore.settings.host = "2.2.2.2" // This is not allowed

Javascript on the other hand has no such stateMachine with regards to settings. You can either get a FirebaseFirestore instance with whatever settings it was previously initialized, or you can initialize it with settings provided it has not been initialized before (there's no telling whether it is btw). The only one you can do is enable the emulator, which you can do after initialization but only before doing any operations.

This difference is the main reason for this major discrepancy. The approach of this library has always been to get the FirebaseFirestore object and change its settings before operations are executed. If you look at the setSettings method on JS pre 1.13.0, you'll realize that it would've crashed if you called the method as it calls the initializationCode after it has already gotten an initialized instance.

I guess it might be possible to essentially remove the state machine logic out of the library, so that you must provide the settings when getting a FirebaseFirestore instance, but this might have unknown side effects and is a breaking change for sure.

The second problem is smaller, and could be solved by omission: Settings on Android are fully readable, on iOS readable except for the cache settings, and on JS (due to initialization mentioned above) not readable at all. As of this PR, we get the settings on Android/iOS (though we miss a few cache settings on iOS) and we track it ourselves for JS.

[nbransby]

Ok well for the second issue I think removing the getter so you can't read settings on any platform is sensible.

As for the first issue, looking at https://firebase.google.com/docs/firestore/manage-data/enable-offline#configure_offline_persistence, I suggest the following:

• make it only possible to set all the settings at once so firestore.settings.host = "1.1.1.1 shouldn't be possible (which will be the case once the getter is removed)
• on JS, since Firebase.firestore.settings = firestoreSettings { ... } needs to call initializeFirestore before getFirestore is called can the js property be lazy so getFirestore is not called doing Firebase.firestore.settings = firestoreSettings { ... }
• if the user attempts to set the settings after doing another operation which means getFirestore has already been called then let the js sdk (and others) throw the relevant error.

[Daeda88]

Agreed:

• Removed settings property in favour of setSettings(builder) and applySettings(settings) methods. This is an api change compared to 1.13.0 but I think we're both fine with that.
• The JS implementation now triggers "setting" the settings if the firestore object has been initialized. This should ensure you get a native crash if you try and set settings after already having initialized the firestore object.

[nbransby]

Lets keep the syntax matching the android sdk with a setter only property rather that two new functions

[Daeda88]

Its not an officially supported feature, so it has some downsides as your link shows. Regardless, Ive implemented it as requested

[Tweener]

Hi 👋
I'm also having the same issue on iOS.
Do you guys have an idea when a new release can be expected?
Thanks!

Btw, love the work you guys have been doing with this library! 🫶