Add tags to calls, including computable tags#9168
Conversation
Originally I was planning on implementing tags on Request only, but that design broke as soon as any interceptor used Request.newBuilder() to create a copy of the Request - the instance that received the tags might not be the instance that needed them. Instead I'm introducing this behavior on the Call object, which behaves less like a value. I expect that users' expected lifetimes of tags will work naturally this way. It is an API hazard that we have tags on both Requests and Calls. Perhaps if given the opportunity to do it over, I'd omit support for tags on Requests.
0808bca to
4e62334
Compare
| fun <T : Any> tag( | ||
| type: KClass<T>, | ||
| computeIfAbsent: () -> T, | ||
| ): T |
There was a problem hiding this comment.
We have this law in Canada where packaged goods need to have both French and English on the label.
Offering both a Kotlin KClass version and a Java Class version of these APIs has the same vibes. I don’t want one set of users to feel diminished vs. the other.
There was a problem hiding this comment.
Oh and if it’s unclear, I believe the french word for Nacho is Nacho. @oldergod can confirm.
| * The tags on a call are seeded from the [request tags][Request.tag]. This set will grow if new | ||
| * tags are computed. | ||
| */ | ||
| fun <T : Any> tag(type: KClass<T>): T? |
There was a problem hiding this comment.
Can this form be a reified type without a param?
There was a problem hiding this comment.
If I do it as an extension function. On an interface it’s not possible ’cause reified requires inline.
There was a problem hiding this comment.
This surprised me when I went to use it.
There was a problem hiding this comment.
The lack of a reified inline function?
There was a problem hiding this comment.
Yep, needing to pass the class param
| * Create a new, identical call to this one which can be enqueued or executed even if this call | ||
| * has already been. | ||
| * | ||
| * The tags on the returned call will equal the tags as on [request]. Any tags that were computed |
| public abstract fun isCanceled ()Z | ||
| public abstract fun isExecuted ()Z | ||
| public abstract fun request ()Lokhttp3/Request; | ||
| public abstract fun tag (Ljava/lang/Class;)Ljava/lang/Object; |
There was a problem hiding this comment.
This seems like a disruptively breaking change. Does the interface default mechanism help?
There was a problem hiding this comment.
Agreed that it’s disruptive.
The normal use-case of this interface is that it’s implemented by OkHttp, and called by our users. In this normal case there’s no problem, as the implementations include the new functions.
The decorator use case is dangerous: users create their own Call implementations that delegate to our implementation, and users’ decorating implementations won’t implement these functions. But I also don’t expect such users to introduce new calls to tag() before they re-compile their Call implementations.
The most most dangerous case is when a third-party library decorates Call, and users introduce new calls to tag() without updating such a library. I will document this risk in our changelog!
Originally I was planning on implementing tags on
Request only, but that design broke as soon as any
interceptor used Request.newBuilder() to create a
copy of the Request - the instance that received the
tags might not be the instance that needed them.
Instead I'm introducing this behavior on the Call
object, which behaves less like a value. I expect
that users' expected lifetimes of tags will work
naturally this way.
It is an API hazard that we have tags on both
Requests and Calls. Perhaps if given the opportunity
to do it over, I'd omit support for tags on Requests.