upgrade to okhttp3 #220
upgrade to okhttp3 #220
Conversation
also upgrade okio okhttp 2.x is obsolete, and according to https://github.com/square/okhttp/blob/master/CHANGELOG.md#version-300-rc1 there should be no risk to upgrade.
Codecov Report
@@ Coverage Diff @@
## master #220 +/- ##
============================================
+ Coverage 71.24% 71.26% +0.01%
Complexity 724 724
============================================
Files 148 148
Lines 3071 3073 +2
Branches 176 176
============================================
+ Hits 2188 2190 +2
Misses 837 837
Partials 46 46
Continue to review full report at Codecov.
|
|
@martina-if Does this makes sense? Or you have tried but found some big issue? |
There was a problem hiding this comment.
👍 but is there a CHANGELOG for the project where you could make note of this change?
Since okhttp3 uses a different groupId and a different package name, it's possible for you to have (or use) both okhttp3 and okhttp2 on the classpath at the same time - so a user of apollo's http-client module might still be referencing okhttp2 code in their code after upgrading to this version (if for some reason they used okhttp-client but still had a need to refer to some okhttp-specific code).
|
@mattnworb that makes perfect sense! not sure how this kind of change has been tracked usually in this project. I'm not a frequent committer here. :D |
|
@mattnworb Projects that access okhttp2 directly should declare the direct dependency on okhttp2 instead of relying on the indirect dependency through apollo. Projects that do not declare a direct dependency might get a compilation error when they upgrade apollo and they can then either add the direct dependency or (preferably) also switch to using okhttp3. This seems fair to me. Am I missing something? Wdyt? |
|
Right, they should declare a direct dependency. My question was just if there is a way to document this change to help make it obvious at upgrade-time, especially for anyone who doesn't do the right thing. I don't see a CHANGELOG in the repo but seems like the main maintainers have been able to document each release on the releases page just fine without one, so perhaps it isn't that important. |
|
So ok to merge? @pettermahlen |
|
@mattnworb Yeah, ideally maven would be a bit more helpful in these situations. To digress a little bit, I guess java modules would have prevented this situation to begin with. Anyway, I guess we should at least mention the okhttp change on the releases page. |
|
@honnix Thank you for the PR! Master branch is for apollo 2.0 version. We don't have that release planned in the near future, did you mean to have it for the next 1.x release? In that case you should duplicate the PR against branch Regarding the compilation errors. I think that since they are compilation errors this is reasonable. It doesn't really change the apollo API in any way and it's better than runtime missing links. |
|
This can be for both 2.0 and 1.x I think. How about getting it into 2.0 and back port to 1.x next release? |
|
@honnix Yes, please, duplicate it to the other branch. I'll merge this one. |
also upgrade okio
okhttp 2.x is obsolete, and according to https://github.com/square/okhttp/blob/master/CHANGELOG.md#version-300-rc1
there should be no risk to upgrade.