Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate client.identify() calls to use CompletableFuture<T> #277

Open
erawhctim opened this issue Sep 16, 2024 · 3 comments
Open

Migrate client.identify() calls to use CompletableFuture<T> #277

erawhctim opened this issue Sep 16, 2024 · 3 comments

Comments

@erawhctim
Copy link

Is your feature request related to a problem? Please describe.
It'd be nice if the LD SDK used CompletableFuture<T> (or if the internal Future impl. types LDAwaitFuture<T>, etc, implemented CompletionStage<T>) so that Kotlin apps could leverage the built-in conversion functions from kotlinx.coroutines.future to convert Future's to suspend fn's.

Right now, clients have to do this conversion manually:

suspend fun identifyInternal(): Unit = suspendCancellableCoroutine { continuation ->
  val future = client.identify(/* build a context */)
  continuation.invokeOnCancellation { future.cancel(/* mayInterruptIfRunning = */ true) }

  try {
    future.get()
    continuation.resume(Unit)
  } catch (e: ExecutionException) {
    continuation.resumeWithException(e.cause ?: e)
  } catch (e: InterruptedException) {
    continuation.resumeWithException(e)
  }
}

Ultimately it's not a ton of work to do this manually, but it'd be convenient to eliminate some of this boilerplate conversion code (if supporting CompletableFuture<T> is easy to do on your end).

Describe the solution you'd like
Supporting one of the above types. This would allow Kotlin clients to use kotlinx conversion functions like this:

suspend fun identifyInternal(): Unit {
  return client.identify(/* build a context */).await()
}

Describe alternatives you've considered
Continue doing the hand-written/manual conversion listed above.

Additional context
N/A

@tanderson-ld
Copy link
Contributor

Hi @erawhctim, thank you for the feedback. I'm generally in agreement with you that a built in completable future should be used. Android added support for CompletableFuture in API 24 and our minimum API at the moment is 21. Does that sound correct? If so, I believe we'll have to wait until our min API is at least 24 before integrating this change.

@erawhctim
Copy link
Author

🤦‍♂️ I totally forgot about the Android support side of this problem, yeah that makes sense (and sounds correct to my knowledge)

@tanderson-ld
Copy link
Contributor

No worries! I'll leave this open as an enhancement as I'm sure other users will find your code snippet useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants