Skip to content

Add experimental OkHttp3 websocket client support #19

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

Merged
merged 1 commit into from
Mar 24, 2017

Conversation

rogerhu
Copy link
Contributor

@rogerhu rogerhu commented Mar 23, 2017

We should deprecate TubeSocket if this works.

@mmimeault

jhansche pushed a commit to wondrous-io/ParseLiveQuery-Android that referenced this pull request Mar 23, 2017
This may become unnecessary if/when we move to OkHttp3 (parse-community#19). But for now
it's the only way to see the exact JSON data that is being sent over the
websocket.
@jhansche jhansche mentioned this pull request Mar 23, 2017
rogerhu pushed a commit that referenced this pull request Mar 23, 2017
This may become unnecessary if/when we move to OkHttp3 (#19). But for now
it's the only way to see the exact JSON data that is being sent over the
websocket.

private OkHttp3WebSocketClient(WebSocketClientCallback webSocketClientCallback, URI hostUrl) {
this.webSocketClientCallback = webSocketClientCallback;
client = new OkHttpClient();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we don't reuse the client, maybe we should only create it in the open() method when creating the websocket?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created two constructors now, one default that instantiates one, and another that allows you to pass one in...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm not sure to understand. Maybe my comment wasn't clear.
Why did you add the ability to pass a OkHttpClient? Is it useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes adding a custom OkHttpClient Is useful for troubleshooting and custom options:

https://code.facebook.com/posts/393927910787513/stetho-a-new-debugging-platform-for-android/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks for the clarification.

Copy link
Contributor

@mmimeault mmimeault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small comment

@rogerhu rogerhu force-pushed the okhttp3 branch 2 times, most recently from 087dbd2 to 8c4241d Compare March 24, 2017 05:49
Copy link
Contributor

@mmimeault mmimeault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine, let's try it. Small comment then it's all good IMO.

private final WebSocketClientCallback webSocketClientCallback;
private WebSocket webSocket;
private State state = State.NONE;
private OkHttpClient client;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could make it final.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good thanks

We should deprecate TubeSocket if this works.
@rogerhu rogerhu merged commit 7c1ead1 into parse-community:master Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants