JakeWharton / retrofit2-kotlin-coroutines-adapter

A Retrofit 2 adapter for Kotlin coroutine's Deferred type.
Apache License 2.0
1.97k stars 129 forks source link

Is Deferred<Unit> supported? #2

Closed LouisCAD closed 6 years ago

LouisCAD commented 6 years ago

I recall a snippet of a UnitConverterFactory you posted in an issue. Is is needed to support Deferred<Unit> for 204 No Content or ignored body responses?

JakeWharton commented 6 years ago

Yes. This is only a call adapter. If you want a converter for Unit as a body type, you'll need to install one separately.

LouisCAD commented 6 years ago

@JakeWharton Could be nice to post the snippet in the README since it's likely to be needed by users of this library

JakeWharton commented 6 years ago

I don't think that statement is accurate. I've been using Retrofit in Kotlin for 2 years on many projects and never needed it. Also its utility is independent of your desire to use coroutines with Retrofit and so having it here doesn't make it discoverable for those who aren't using coroutines.

On Wed, Dec 27, 2017 at 2:41 PM Louis CAD notifications@github.com wrote:

@JakeWharton https://github.com/jakewharton Could be nice to post the snippet in the README since it's likely to be needed by users of this library

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/JakeWharton/retrofit2-kotlin-coroutines-adapter/issues/2#issuecomment-354167250, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEUl-5_OHPk5fqWa_NZgo6AkFsC_Bks5tEp1ugaJpZM4RNwZ7 .

PaulWoitaschek commented 6 years ago

I don't understand why this is not provided out of the box. Or am I misunderstanding something? When I have a POST or PATCH isn't Deferred<Unit> always what I want to return here? When using retrofit directly I use a Call<Void>

JakeWharton commented 6 years ago

This is an adapter which provides you Deferred<T>. If you want to use Unit for T, you want a converter.

PaulWoitaschek commented 6 years ago

And how is that possible? I try to write a moshi adapter like

class UnitJsonAdapter {

  @FromJson
  fun fromJson(value: String?): Unit = Unit

  @ToJson
  fun toJson(unit: Unit): String? = null
}

But it fails because the fromJson uses void in bytecode.

JakeWharton commented 6 years ago

https://github.com/square/retrofit/issues/2329#issuecomment-301316873

On Tue, Mar 27, 2018 at 3:12 PM Paul Woitaschek notifications@github.com wrote:

And how is that possible? I try to write a moshi adapter like

class UnitJsonAdapter {

@FromJson fun fromJson(value: String?): Unit = Unit

@ToJson fun toJson(unit: Unit): String? = null }

But it fails because the fromJson uses void in bytecode.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/JakeWharton/retrofit2-kotlin-coroutines-adapter/issues/2#issuecomment-376641486, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEZkgm22QwjwolAGaFfrempVLCe2xks5tio82gaJpZM4RNwZ7 .

PaulWoitaschek commented 6 years ago

Thanks! Also I ask myself if there is no better representation for that. Like RxJava has Completable. Maybe just allow returning a plain Job?

JakeWharton commented 6 years ago

Can you file a new bug about that? Seems like it might work.

On Tue, Mar 27, 2018 at 3:18 PM Paul Woitaschek notifications@github.com wrote:

Thanks! Also I ask myself if there is no better representation for that. Like RxJava has Completable. Maybe just allow returning a plain Job?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/JakeWharton/retrofit2-kotlin-coroutines-adapter/issues/2#issuecomment-376642983, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEVRhOvYhSz2BegmLgPRJhWnHELxsks5tipBpgaJpZM4RNwZ7 .

PaulWoitaschek commented 6 years ago

With that Unit call adapter I have exceptions for Deferred<Unit> (which are printed to std?)

W/System.err: kotlin.KotlinNullPointerException
W/System.err:     at com.jakewharton.retrofit2.adapter.kotlin.coroutines.experimental.CoroutineCallAdapterFactory$BodyCallAdapter$adapt$2.onResponse(CoroutineCallAdapterFactory.kt:102)
        at retrofit2.OkHttpCall$1.onResponse(OkHttpCall.java:123)
W/System.err:     at okhttp3.RealCall$AsyncCall.execute(RealCall.java:153)
W/System.err:     at okhttp3.internal.NamedRunnable.run(NamedRunnable.java:32)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162)
W/System.err:     at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636)
W/System.err:     at java.lang.Thread.run(Thread.java:764)
LouisCAD commented 6 years ago

@PaulWoitaschek Try adding the UnitConverterFactory before other converter factories when you configure your Retrofit.

quentin41500 commented 5 years ago

I'm getting the same error @PaulWoitaschek, how did you fix it?

@LouisCAD, it is currently the first one on the list.

I'm just confused if I even still need the call adapter.

PaulWoitaschek commented 5 years ago

I fixed this by checking for the Unit type:

        override fun onResponse(call: Call<T>, response: Response<T>) {
          if (response.isSuccessful) {
            if (responseType == Unit::class.java) {
              @Suppress("UNCHECKED_CAST")
              deferred.complete(Unit as T)
            } else {
              deferred.complete(response.body()!!)
            }
          } else {
            deferred.completeExceptionally(HttpException(response))
          }
        }

@JakeWharton Will you accept a PR on this?

ashdavies commented 5 years ago

I'm getting the same error output as @PaulWoitaschek, with Retrofit 2.5.0, and including UnitConverterFactory before any others, using Deferred<Response<Unit>> alleviates it.

Is this still an issue? or is it just an incorrect configuration?

roelvandun commented 5 years ago

Also it would help debugging to write to Log.e instead of Log.w

branoduris commented 5 years ago

I'm getting the same error, with build in UnitResponseBodyConverter in Retrofit 2.6.0. I used @PaulWoitaschek fix on response function.