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

chore: update a deprecated usage of resume function Kotlinx coroutines in okhttp-coroutines #8428

Closed
wants to merge 1 commit into from

Conversation

EchoEllet
Copy link

It seems that the project uses a modern version of Kotlin

org-jetbrains-kotlin = "1.9.23"

Which already supports using the new function

image

It seems the old function was experimental before Kotlin 1.9.0 which might be one reason why executeAsync requires @ExperimentalCoroutinesApi annotation

@JakeWharton
Copy link
Collaborator

Should we remove the annotation too then?

@EchoEllet
Copy link
Author

EchoEllet commented Jun 4, 2024

Should we remove the annotation too then?

The version of the okhttp-coroutines module says it's in alpha (5.0.0-alpha.11)

it works just like expected for me, I'm not sure what's missing and why this annotation is used in the first place, my guess is it was required on older versions of Kotlinx coroutines, see this issue: Kotlin/kotlinx.coroutines#4088

It was created by a developer from Square (the same community/organization that developed OkHttp) and it seems to be fixed, I'm still not sure why it's not removed, maybe they use the experimental annotation from Kotlinx coroutines as a way to indicate the functionality is not production ready, subject to change, miss something, have bugs or they have some other reasons

For me, it was working perfectly fine, the code seemed to compile successfully without annotating the function and requiring to mark the functions that are using executeAsync()

Not related issue: using this module makes the bundle size a little bit bigger (compared to the size of the current project I'm working on), there might be unused dependencies or some libraries that are meant to be used in tests only or some other imports, I tried to use this extension function by copying the code to a blank project vs using the extension function from okhttp-coroutines
image

The JAR file size is smaller when copying the code of the extension function inside this module, since now the extension function executeAsync is stable, it's possible to publish the okhttp with okhttp-coroutines with the same version

If you go to

@ExperimentalCoroutinesApi // resume with a resource cleanup.

The reason why this annotation is used might be that it uses a Kotlinx coroutines function that's needed to use response.closeQuietly()

@yschimke
Copy link
Collaborator

yschimke commented Jul 6, 2024

Doesn't this require bumping to the 1.9.0-RC of Kotlinx coroutines

https://github.com/Kotlin/kotlinx.coroutines/releases/tag/1.9.0-RC

Promoted CancellableContinuation.resume with an onCancellation lambda to stable, providing extra arguments to the lambda

@EchoEllet
Copy link
Author

Doesn't this require bumping to the 1.9.0-RC of Kotlinx coroutines

https://github.com/Kotlin/kotlinx.coroutines/releases/tag/1.9.0-RC

Promoted CancellableContinuation.resume with an onCancellation lambda to stable, providing extra arguments to the lambda

It does, though this is the minimum supported version by Kotlin 2.0.0. Most projects are already working on migrating to use K2.

Otherwise, we would need to wait for a while before increasing the minimum version.

This change might need to be done to OkHttp 5.0.0 instead. Closing this PR.

@EchoEllet EchoEllet closed this Jul 6, 2024
@yschimke
Copy link
Collaborator

yschimke commented Jul 6, 2024

We should follow Okio, even if not strictly neccesary. I think they are on 1.9.21

Pending PR square/okio#1483

@EchoEllet EchoEllet deleted the patch-1 branch July 6, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants