Use Call.Factory instead of OkHttpClient directly#293
Conversation
wzieba
left a comment
There was a problem hiding this comment.
Thanks! LGTM
If this is considered a deal-breaker, I can revert the parameter name change. However, I do not think it is a significant issue, as this dependency is typically instantiated only once in most applications, making the migration effort minimal. We can also explicitly call this out in the changelog.
I think it's totally fine - IMO it's not a deal breaker, especially for internal-only library.
Just for a reference, would you mind linking on this PR, a PR from Pocket Casts that will introduce lazy initialization of OkHttpClient ? I think it can be a valuable resource for any consumer of the lib.
Do you mean for me to make a PR with changes in Pocket Casts first against the build from the 7985925 commit and then link it here before merging this PR? If yes, then no problem. |
|
I think that we can do this after integrating the new, stable release to Pocket Casts - not necceseriely against the build from 7985925 commit 👍 Let me release new version of Tracks now |
|
So there are two related changes in Pocket Casts:
|
Using
Call.Factoryinstead ofOkHttpClientallows library consumers to instantiateOkHttpClientlazily if desired. This is important becauseOkHttpClientinitialization can take upwards of 100 ms on some Android devices, potentially causing ANRs or slowing down app startup.This change is binary-compatible, as
Call.Factoryis a superinterface ofOkHttpClient. However, it may be source-incompatible for Kotlin consumers that rely on named parameters, since the constructor parameter was renamed fromokhttpClienttocallFactory.If this is considered a deal-breaker, I can revert the parameter name change. However, I do not think it is a significant issue, as this dependency is typically instantiated only once in most applications, making the migration effort minimal. We can also explicitly call this out in the changelog.