Skip to content

Conversation

rlazo
Copy link
Collaborator

@rlazo rlazo commented Sep 19, 2019

No description provided.

* use the type `T`, and not `? extends T`.
*/
inline fun <reified T> DataSnapshot.getValue(): T? {
var genericTypeIndicator = object : GenericTypeIndicator<T>() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe use val instead of var here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!. Fixed

@rlazo
Copy link
Collaborator Author

rlazo commented Sep 19, 2019

/retest

* use the type `T`, and not `? extends T`.
*/
inline fun <reified T> DataSnapshot.getValue(): T? {
val genericTypeIndicator = object : GenericTypeIndicator<T>() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth inlining into getValue call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

@msasikanth
Copy link

msasikanth commented Sep 20, 2019

Also add an extension supporting Kotlin coroutines? I can provide a PR for this separately.

suspend fun <T : Any> Query.await(): T? {
    return suspendCancellableCoroutine { continuation ->
        val eventListener = object : ValueEventListener {
            override fun onDataChange(dataSnap: DataSnapshot) {
                val type = object : GenericTypeIndicator<T>() {}
                continuation.resume(dataSnap.getValue(type))
            }

            override fun onCancelled(dbError: DatabaseError) {
                continuation.resumeWithException(dbError.toException())
            }
        }
        continuation.invokeOnCancellation {
            removeEventListener(eventListener)
        }
        addListenerForSingleValueEvent(eventListener)
    }
}

@msasikanth
Copy link

msasikanth commented Sep 21, 2019

Also add an extension supporting Kotlin coroutines? I can provide a PR for this separately.

suspend fun <T : Any> Query.await(): T? {
    return suspendCancellableCoroutine { continuation ->
        val eventListener = object : ValueEventListener {
            override fun onDataChange(dataSnap: DataSnapshot) {
                val type = object : GenericTypeIndicator<T>() {}
                continuation.resume(dataSnap.getValue(type))
            }

            override fun onCancelled(dbError: DatabaseError) {
                continuation.resumeWithException(dbError.toException())
            }
        }
        continuation.invokeOnCancellation {
            removeEventListener(eventListener)
        }
        addListenerForSingleValueEvent(eventListener)
    }
}

This extension may not work as expected since type is being erased at runtime. In order to fix that class type should be passed as parameter of the function or make the function inline and use reified. I don't think inlining this function is a good idea. So I will be passing the type as a parameter similar to the getValue(T) in DataSnapshot class.

suspend fun <T : Any> Query.await(type: Class<T>): T? {
    return suspendCancellableCoroutine { continuation ->
        val eventListener = object : ValueEventListener {
            override fun onDataChange(dataSnap: DataSnapshot) {
                continuation.resume(dataSnap.getValue(type))
            }

            override fun onCancelled(dbError: DatabaseError) {
                continuation.resumeWithException(dbError.toException())
            }
        }
        continuation.invokeOnCancellation {
            removeEventListener(eventListener)
        }
        addListenerForSingleValueEvent(eventListener)
    }
}

@rlazo
Copy link
Collaborator Author

rlazo commented Sep 23, 2019

Also add an extension supporting Kotlin coroutines? I can provide a PR for this separately.

suspend fun <T : Any> Query.await(): T? {
    return suspendCancellableCoroutine { continuation ->
        val eventListener = object : ValueEventListener {
            override fun onDataChange(dataSnap: DataSnapshot) {
                val type = object : GenericTypeIndicator<T>() {}
                continuation.resume(dataSnap.getValue(type))
            }

            override fun onCancelled(dbError: DatabaseError) {
                continuation.resumeWithException(dbError.toException())
            }
        }
        continuation.invokeOnCancellation {
            removeEventListener(eventListener)
        }
        addListenerForSingleValueEvent(eventListener)
    }
}

This extension may not work as expected since type is being erased at runtime. In order to fix that class type should be passed as parameter of the function or make the function inline and use reified. I don't think inlining this function is a good idea. So I will be passing the type as a parameter similar to the getValue(T) in DataSnapshot class.

suspend fun <T : Any> Query.await(type: Class<T>): T? {
    return suspendCancellableCoroutine { continuation ->
        val eventListener = object : ValueEventListener {
            override fun onDataChange(dataSnap: DataSnapshot) {
                continuation.resume(dataSnap.getValue(type))
            }

            override fun onCancelled(dbError: DatabaseError) {
                continuation.resumeWithException(dbError.toException())
            }
        }
        continuation.invokeOnCancellation {
            removeEventListener(eventListener)
        }
        addListenerForSingleValueEvent(eventListener)
    }
}

It's an interesting suggestion @msasikanth If you could provide a PR it would be great. Please, do keep in mind that changes in the API are subject to internal approval. Thanks!

@rlazo rlazo merged commit b6f4ce0 into master Sep 23, 2019
@rlazo rlazo deleted the rl.rtdb-ktx branch September 23, 2019 19:25
@thatfiredev
Copy link
Member

@msasikanth Please let me know when you add that extension so that I can include it on #841 :)

@msasikanth
Copy link

@msasikanth Please let me know when you add that extension so that I can include it on #841 :)

Absolutely, once I figure how to setup the project in Android Studio properly. I am having sync and test run issues with it.

@msasikanth
Copy link

@rosariopfernandes , @rlazo I have created the PR for the extension

#862

@firebase firebase locked and limited conversation to collaborators Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes Override cla size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants