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

Upgrade to OkHttp 3.8.0 and migrate to new websocket api #11835

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@AndrewJack
Contributor

AndrewJack commented Jan 11, 2017

Motivation

  • Keeping a core dependency up to date
  • Fixes: #11680
  • Resolves dependency conflicts

3.5.0 - Change log
3.6.0 - Change log
3.7.0 - Change log
3.8.0 - Change log

Test plan

  • Tests pass on CI
  • Tested with the example projects in JS remote debug mode

iOS only ping

The sendPing public API has been removed. In 3.5.0 OkHttp will automatically ping and send a callback to onClose if not connected.

Reference: square/okhttp#2902 (comment)

I guessing this PR will be dependent on Facebook upgrading OkHttp on their side?
cc @bestander

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 11, 2017

By analyzing the blame information on this pull request, we identified @lexs and @frantic to be potential reviewers.

facebook-github-bot commented Jan 11, 2017

By analyzing the blame information on this pull request, we identified @lexs and @frantic to be potential reviewers.

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack Jan 11, 2017

Contributor

Failing for the same reason as master

Contributor

AndrewJack commented Jan 11, 2017

Failing for the same reason as master

@wildseansy

This comment has been minimized.

Show comment
Hide comment
@wildseansy

wildseansy Jan 17, 2017

Contributor

Yay 🎉 happy to see tests are passing for this one. Would love to see this in the upcoming release

Contributor

wildseansy commented Jan 17, 2017

Yay 🎉 happy to see tests are passing for this one. Would love to see this in the upcoming release

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Jan 18, 2017

Contributor

I'll ping the internal group and ask someone to merge.
Because FB uses monorepo and all dependencies are flat updating a core library may take some time

Contributor

bestander commented Jan 18, 2017

I'll ping the internal group and ask someone to merge.
Because FB uses monorepo and all dependencies are flat updating a core library may take some time

@wildseansy wildseansy referenced this pull request Jan 18, 2017

Closed

Okhttp 3.5.0 support #11698

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack Jan 30, 2017

Contributor

Should probably upgrade to 3.6 now. There is a lot of bug fixes https://github.com/square/okhttp/blob/master/CHANGELOG.md#version-360

Contributor

AndrewJack commented Jan 30, 2017

Should probably upgrade to 3.6 now. There is a lot of bug fixes https://github.com/square/okhttp/blob/master/CHANGELOG.md#version-360

@AndrewJack AndrewJack changed the title from Upgrade to OkHttp 3.5.0 and migrate to new websocket api to Upgrade to OkHttp 3.6.0 and migrate to new websocket api Jan 30, 2017

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack Jan 30, 2017

Contributor

Upgraded to 3.6.0 and fixed conflicts

Contributor

AndrewJack commented Jan 30, 2017

Upgraded to 3.6.0 and fixed conflicts

@rodperottoni

This comment has been minimized.

Show comment
Hide comment
@rodperottoni

rodperottoni Feb 1, 2017

I started implementing a wrapper for the Socket.IO lib yesterday and turns out it needs the latest version of OkHTTP to work. Requiring the latest version of it in my Module was simply not working (multiple dex files error), and therefore seeing this PR the same day was a huge coincidence.
How much time does it usually take for PRs to be accepted by FB ?
Thanks @AndrewJack for this PR.

rodperottoni commented Feb 1, 2017

I started implementing a wrapper for the Socket.IO lib yesterday and turns out it needs the latest version of OkHTTP to work. Requiring the latest version of it in my Module was simply not working (multiple dex files error), and therefore seeing this PR the same day was a huge coincidence.
How much time does it usually take for PRs to be accepted by FB ?
Thanks @AndrewJack for this PR.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Feb 1, 2017

Contributor

Just to adjust your expectations, merging this PR is some work because all apps that use okhttp and react-native need to be updated at FB.
Looking at these changes the update is not a simple drop in replacement, quite a few public API changed and all this needs to be coded and tested.
Someone would have to volunteer their time to do the changes across many apps.

Someone will definitely do it eventually but the fact that okhttp makes a minor release every few weeks does not help.

Contributor

bestander commented Feb 1, 2017

Just to adjust your expectations, merging this PR is some work because all apps that use okhttp and react-native need to be updated at FB.
Looking at these changes the update is not a simple drop in replacement, quite a few public API changed and all this needs to be coded and tested.
Someone would have to volunteer their time to do the changes across many apps.

Someone will definitely do it eventually but the fact that okhttp makes a minor release every few weeks does not help.

@rodperottoni

This comment has been minimized.

Show comment
Hide comment
@rodperottoni

rodperottoni Feb 1, 2017

@bestander Oh, good to know! Thanks for the clarification.

rodperottoni commented Feb 1, 2017

@bestander Oh, good to know! Thanks for the clarification.

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack Mar 3, 2017

Contributor

I've resolved the latest conflicts if anyone at Facebook has time to merge this into your mono repo?

Contributor

AndrewJack commented Mar 3, 2017

I've resolved the latest conflicts if anyone at Facebook has time to merge this into your mono repo?

@felipecsl

This comment has been minimized.

Show comment
Hide comment
@felipecsl

felipecsl Apr 6, 2017

Contributor

Would like to see this merged soon if possible.

Contributor

felipecsl commented Apr 6, 2017

Would like to see this merged soon if possible.

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack Apr 7, 2017

Contributor

@felipecsl Latest conflicts have been resolved.

Just waiting for Facebook to have time to update this. Not sure who's best to ping, maybe @hramos?

Contributor

AndrewJack commented Apr 7, 2017

@felipecsl Latest conflicts have been resolved.

Just waiting for Facebook to have time to update this. Not sure who's best to ping, maybe @hramos?

@hramos

This comment has been minimized.

Show comment
Hide comment
@hramos

hramos Apr 7, 2017

Contributor

Pinging @ericvicenti who is oncall this week

Contributor

hramos commented Apr 7, 2017

Pinging @ericvicenti who is oncall this week

@YuliaGrigorieva

This comment has been minimized.

Show comment
Hide comment
@YuliaGrigorieva

YuliaGrigorieva Apr 11, 2017

Hi,

Is there any visibility when this pull request will be merged?

Thanks.

YuliaGrigorieva commented Apr 11, 2017

Hi,

Is there any visibility when this pull request will be merged?

Thanks.

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Apr 13, 2017

Contributor

As I commented above this is not an easy to merge PR because API changed in a breaking way.
Can someone think of a solution without having to update every call to okhttp?

Contributor

bestander commented Apr 13, 2017

As I commented above this is not an easy to merge PR because API changed in a breaking way.
Can someone think of a solution without having to update every call to okhttp?

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack Apr 13, 2017

Contributor

The only way I can think of is to create a OkHttp wrapper library, that is published to maven/jcenter, which has the same interface between versions.

That library would have versions v3.4.1 and v3.6.0, react native by default would use v3.4.1. Developers that want the later version can add v3.6.0 to their dependencies to update the transitive dependency.

However that'll be a very large task and maintenance overhead for a library so rooted in the React Native implementation, but if thats what the OSS project has to do, then maybe that's our only option.

Contributor

AndrewJack commented Apr 13, 2017

The only way I can think of is to create a OkHttp wrapper library, that is published to maven/jcenter, which has the same interface between versions.

That library would have versions v3.4.1 and v3.6.0, react native by default would use v3.4.1. Developers that want the later version can add v3.6.0 to their dependencies to update the transitive dependency.

However that'll be a very large task and maintenance overhead for a library so rooted in the React Native implementation, but if thats what the OSS project has to do, then maybe that's our only option.

@michaelr524

This comment has been minimized.

Show comment
Hide comment
@michaelr524

michaelr524 Apr 13, 2017

How about RN embedding it's dep version of okhttp by renaming the package?

michaelr524 commented Apr 13, 2017

How about RN embedding it's dep version of okhttp by renaming the package?

@bestander

This comment has been minimized.

Show comment
Hide comment
@bestander

bestander Apr 13, 2017

Contributor

@AndrewJack are you in the core contributors group?
Maybe you could bring it up for discussion?
This does not look like a task with an easy solution:(

Contributor

bestander commented Apr 13, 2017

@AndrewJack are you in the core contributors group?
Maybe you could bring it up for discussion?
This does not look like a task with an easy solution:(

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack Apr 13, 2017

Contributor

 @bestander I am, I'll post something this afternoon. soon...

Contributor

AndrewJack commented Apr 13, 2017

 @bestander I am, I'll post something this afternoon. soon...

@felipecsl

This comment has been minimized.

Show comment
Hide comment
@felipecsl

felipecsl Apr 13, 2017

Contributor

Sounds like shading the dependency with jarjar would be the simplest solution, no?

Contributor

felipecsl commented Apr 13, 2017

Sounds like shading the dependency with jarjar would be the simplest solution, no?

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack Apr 13, 2017

Contributor

@felipecsl shading would only solve dependency conflicts.

We would still be stuck using v3.4.1 for react native networking and miss out on the bug fixes from 3.5 and 3.6.

Neither solution is ideal.

Contributor

AndrewJack commented Apr 13, 2017

@felipecsl shading would only solve dependency conflicts.

We would still be stuck using v3.4.1 for react native networking and miss out on the bug fixes from 3.5 and 3.6.

Neither solution is ideal.

@Chubacca

This comment has been minimized.

Show comment
Hide comment
@Chubacca

Chubacca Apr 13, 2017

If it takes significantly less time to use shading it might be nice for those of us who are urgently waiting on a fix for just the dependency conflicts.

Chubacca commented Apr 13, 2017

If it takes significantly less time to use shading it might be nice for those of us who are urgently waiting on a fix for just the dependency conflicts.

@felipecsl

This comment has been minimized.

Show comment
Hide comment
@felipecsl

felipecsl Apr 13, 2017

Contributor

Indeed, this would unblock and allow users to move on to 3.6 while Facebook can figure out how/when to upgrade at its own pace, no rush.

Contributor

felipecsl commented Apr 13, 2017

Indeed, this would unblock and allow users to move on to 3.6 while Facebook can figure out how/when to upgrade at its own pace, no rush.

@hramos

This comment has been minimized.

Show comment
Hide comment
@hramos

hramos Apr 20, 2017

Contributor

We're aware of the interest in this issue. Please avoid adding new comments that are simply "+1"s so we can make sure every interested party can follow the discussion without any added noise. As an alternative, I suggest using reactions to add a thumbs up to the original post in the thread. Thanks!

Contributor

hramos commented Apr 20, 2017

We're aware of the interest in this issue. Please avoid adding new comments that are simply "+1"s so we can make sure every interested party can follow the discussion without any added noise. As an alternative, I suggest using reactions to add a thumbs up to the original post in the thread. Thanks!

@@ -53,7 +50,7 @@
private @Nullable JSDebuggerCallback mConnectCallback;
private final AtomicInteger mRequestID = new AtomicInteger();
private final ConcurrentHashMap<Integer, JSDebuggerCallback> mCallbacks =
new ConcurrentHashMap<>();
new ConcurrentHashMap<>();
public void connect(String url, JSDebuggerCallback callback) {
if (mHttpClient != null) {

This comment has been minimized.

@petterh

petterh Apr 25, 2017

Contributor

I believe mHttpClient can be converted into a local variable.

@petterh

petterh Apr 25, 2017

Contributor

I believe mHttpClient can be converted into a local variable.

This comment has been minimized.

@AndrewJack

AndrewJack May 1, 2017

Contributor

Don't think so, this prevents anyone from connecting while it's already connected

@AndrewJack

AndrewJack May 1, 2017

Contributor

Don't think so, this prevents anyone from connecting while it's already connected

This comment has been minimized.

@petterh

petterh May 2, 2017

Contributor

Yes, you're right, of course. (I was looking at ReconnectingWebSocket which is quite similar -- but uses a boolean field mClosed to prevent connection if it's already connected.)

@petterh

petterh May 2, 2017

Contributor

Yes, you're right, of course. (I was looking at ReconnectingWebSocket which is quite similar -- but uses a boolean field mClosed to prevent connection if it's already connected.)

This comment has been minimized.

@AndrewJack

AndrewJack May 2, 2017

Contributor

ok, I'll check that.

@AndrewJack

AndrewJack May 2, 2017

Contributor

ok, I'll check that.

petterh added a commit to petterh/react-native that referenced this pull request Apr 25, 2017

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack May 1, 2017

Contributor

Sorry for the delay, I was on holiday. https://www.facebook.com/groups/reactnativeoss/permalink/1675140666115894/

cc @bestander

https://github.com/square/okhttp/blob/master/CHANGELOG.md#version-370 is out now if we do reach a conclusion we should update to that.

Contributor

AndrewJack commented May 1, 2017

Sorry for the delay, I was on holiday. https://www.facebook.com/groups/reactnativeoss/permalink/1675140666115894/

cc @bestander

https://github.com/square/okhttp/blob/master/CHANGELOG.md#version-370 is out now if we do reach a conclusion we should update to that.

@StevePotter

This comment has been minimized.

Show comment
Hide comment
@StevePotter

StevePotter May 8, 2017

Contributor

Yeah you might as well go to 3.7. If you need help let me know. Thanks for this!

Contributor

StevePotter commented May 8, 2017

Yeah you might as well go to 3.7. If you need help let me know. Thanks for this!

@AndrewJack AndrewJack changed the title from Upgrade to OkHttp 3.6.0 and migrate to new websocket api to Upgrade to OkHttp 3.8.0 and migrate to new websocket api May 17, 2017

@emilsjolander

This comment has been minimized.

Show comment
Hide comment
@emilsjolander

emilsjolander May 18, 2017

Contributor

I needed 3.8.0 internally and made the update without knowing about this PR. Sorry :/ Thank you for pushing for this! and thanks @bestander for bringing this to my attention.

okhttp 3.8.0 in RN should hit master later this week unless we run into any unexpected failures.

Contributor

emilsjolander commented May 18, 2017

I needed 3.8.0 internally and made the update without knowing about this PR. Sorry :/ Thank you for pushing for this! and thanks @bestander for bringing this to my attention.

okhttp 3.8.0 in RN should hit master later this week unless we run into any unexpected failures.

@AndrewJack

This comment has been minimized.

Show comment
Hide comment
@AndrewJack

AndrewJack May 18, 2017

Contributor

@emilsjolander Great news! At least it has been updated. 👍

Contributor

AndrewJack commented May 18, 2017

@emilsjolander Great news! At least it has been updated. 👍

@emilsjolander

This comment has been minimized.

Show comment
Hide comment
@emilsjolander

emilsjolander May 18, 2017

Contributor

Landed. 93a1d59

Contributor

emilsjolander commented May 18, 2017

Landed. 93a1d59

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment