Remove Native classes from Public API #555
Conversation
| public final fun removeValue (Lkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public final fun setEncodedValue (Ljava/lang/Object;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; |
There was a problem hiding this comment.
Because we have a bunch of inline methods that need access to the Native classes. Though those had been set as internal and annotated with @PublishedApi, this results in them still poping up in the API specification.
So it's essentially choosing between having the Native... classes in the api or these methods. I want for these because it makes the API "smaller"
There was a problem hiding this comment.
To clarify: these methods are also marked as@PublishedApi internal fun and the access the native classes inside their code. As a result the Native classes no longer need to be Published, but the method does because public inline methods can only call publish/Published code
| public final fun setValue (Lkotlinx/serialization/SerializationStrategy;Ljava/lang/Object;Lkotlin/jvm/functions/Function1;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public final fun setValue (Lkotlinx/serialization/SerializationStrategy;Ljava/lang/Object;ZLkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public static synthetic fun setValue$default (Ldev/gitlive/firebase/database/OnDisconnect;Lkotlinx/serialization/SerializationStrategy;Ljava/lang/Object;Lkotlin/jvm/functions/Function1;Lkotlin/coroutines/Continuation;ILjava/lang/Object;)Ljava/lang/Object; | ||
| public final fun updateChildren (Ljava/util/Map;Lkotlin/jvm/functions/Function1;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public final fun updateChildren (Ljava/util/Map;ZLkotlin/coroutines/Continuation;)Ljava/lang/Object; | ||
| public static synthetic fun updateChildren$default (Ldev/gitlive/firebase/database/OnDisconnect;Ljava/util/Map;Lkotlin/jvm/functions/Function1;Lkotlin/coroutines/Continuation;ILjava/lang/Object;)Ljava/lang/Object; | ||
| public final fun updateEncodedChildren (Ldev/gitlive/firebase/internal/EncodedObject;Lkotlin/coroutines/Continuation;)Ljava/lang/Object; |
|
Understood, do these @PublishedApi function appear in the kdocs I wonder. Does this change also mean the ios, android and is properties don't need to be extensions? |
|
Afaik internal methods marked with @PublishedApi are not accessible from kotlin, but they are from Java. So I don't think they need Kdoc. As for the extensions for android/ios/JS, they are still needed as they extend the classes that are in common code, e.g 'Query', to provide access to the Native classes (e.g. 'NativeQuery'). It used to be that these native classes, usually Typealiases, where public as well, but with this change all of that is internal. So in order to still support construction with platform specific code I added some 'invoke' methods to the companion objects though extension as well. That means, you can still do 'Query(androidQuery)' on platform code, but no longer from common code and it requires an import of 'dev.gitlive.firebase.firestore.invoke'. |
| @@ -34,14 +34,16 @@ public expect val Firebase.firestore: FirebaseFirestore | |||
| /** Returns the [FirebaseFirestore] instance of a given [FirebaseApp]. */ | |||
| public expect fun Firebase.firestore(app: FirebaseApp): FirebaseFirestore | |||
|
|
|||
| public expect class NativeFirebaseFirestore | |||
| internal expect class NativeFirebaseFirestore | |||
|
|
|||
| public class FirebaseFirestore internal constructor(private val wrapper: NativeFirebaseFirestoreWrapper) { | |||
There was a problem hiding this comment.
Leaving comments to illustrate what I mean.
This class in common is the class users interact with. It is not an expect actual class and as such, to have the android/ios/js accessors, we need to write extension values on the platforms
|
|
||
| // Important to leave this as a get property since on JS it is initialized lazily | ||
| public val native: NativeFirebaseFirestore get() = wrapper.native | ||
| internal val native: NativeFirebaseFirestore get() = wrapper.native |
There was a problem hiding this comment.
This used to be a common level alternative to android/ios/jsbut it is now internal, becauseNativeFirebaseFirestore` is now internal
| public constructor(native: NativeFirebaseFirestore) : this(NativeFirebaseFirestoreWrapper(native)) | ||
| public companion object {} | ||
|
|
||
| internal constructor(native: NativeFirebaseFirestore) : this(NativeFirebaseFirestoreWrapper(native)) |
There was a problem hiding this comment.
Since NativeFirebaseFirestore is now internal, the constuctor has to be as well
|
|
||
| public class FirebaseFirestore internal constructor(private val wrapper: NativeFirebaseFirestoreWrapper) { | ||
|
|
||
| public constructor(native: NativeFirebaseFirestore) : this(NativeFirebaseFirestoreWrapper(native)) | ||
| public companion object {} |
There was a problem hiding this comment.
To circumvent https://github.com/GitLiveApp/firebase-kotlin-sdk/pull/555/files#r1655371860 we introduce a companion object
| @@ -34,14 +34,16 @@ public expect val Firebase.firestore: FirebaseFirestore | |||
| /** Returns the [FirebaseFirestore] instance of a given [FirebaseApp]. */ | |||
| public expect fun Firebase.firestore(app: FirebaseApp): FirebaseFirestore | |||
|
|
|||
| public expect class NativeFirebaseFirestore | |||
| internal expect class NativeFirebaseFirestore | |||
There was a problem hiding this comment.
This is a typealias to FIRFirestore(ios) and com.google.firebase.firestore.FirebaseFirestore(android) or the wrapper on Javascript. To remove it from the public API, it is now internal but this has a cascading effect on
https://github.com/GitLiveApp/firebase-kotlin-sdk/pull/555/files#r1655371860
https://github.com/GitLiveApp/firebase-kotlin-sdk/pull/555/files#r1655371283
|
|
||
| public val FirebaseFirestore.android: NativeFirebaseFirestore get() = native | ||
| public operator fun FirebaseFirestore.Companion.invoke(android: AndroidFirebaseFirestore): FirebaseFirestore = FirebaseFirestore(android) |
There was a problem hiding this comment.
With the public construcor gone (see https://github.com/GitLiveApp/firebase-kotlin-sdk/pull/555/files#r1655371860), in order to still support FirebaseFirestore(android: AndroidFirebaseFirestore) we can add an extension to the companion object https://github.com/GitLiveApp/firebase-kotlin-sdk/pull/555/files#r1655372358
This maintains the same syntax, but since it is an extension method, it does require an explicit import
|
ok so ill finish the breaking change of converting all .js, .android, .ios properties to extensions in #504 |
Yes they dont need a docblock but I still wonder if dokka will publish them in the api docs |
Fixes based on comment #550 (comment)