Implement WebSocket module for Android. Fixes #2837 #2839

satya164 · 2015-09-18T14:31:30Z

The JavaScript code for Android is same as the iOS counterpart, I just added few new lines and used arrow functions instead of binding this.

foghina · 2015-09-18T15:16:31Z

ReactAndroid/build.gradle

@@ -240,6 +240,7 @@ dependencies {
    compile 'com.squareup.okhttp:okhttp-ws:2.4.0'
    compile 'com.squareup.okio:okio:1.5.0'
    compile 'org.webkit:android-jsc:r174650'
+    compile "org.java-websocket:Java-WebSocket:1.3.0"


Since we already include okhttp-ws a few lines above this, we should use that instead of adding another dependency.

Thanks, I'll work on migrating the code to use okhttp ws.

Agree with @foghina's suggestion. If you find that Java-WebSocket works better though you could always publish your own module to npm instead though.

foghina · 2015-09-18T15:19:05Z

Thanks for the pull request!

Please see my note about using okhttp-ws instead of org.java-websocket. Which means WebSocketModule will need to be changed as well. But the general approach seems fine.

foghina · 2015-09-18T15:20:05Z

Regarding tests, we have not opensourced those yet so we don't have a story around it. Stay tuned! (cc @mkonicek)

satya164 · 2015-09-18T16:21:49Z

@foghina Thanks. looking forward to it :)

satya164 · 2015-09-18T16:50:24Z

@foghina From the README on the okhttp-ws page

Note: This module's API should be considered experimental and may be subject to breaking changes in future releases.

Currently it's used to connect to Chrome debugger I think. The docs don't have anything and the API is also subject to change. Should I migrate it to use okhttp-ws ? I'm a bit skeptical.

mkonicek · 2015-09-18T18:55:01Z

Yes please migrate to okhttp-ws, if there are breaking changes we'll migrate.

See how it's used here: https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/bridge/JSDebuggerWebSocketClient.java

satya164 · 2015-09-18T20:44:08Z

@mkonicek Ok, I'll migrate to okhttp-ws and update the pull request accordingly.

satya164 · 2015-09-20T00:48:23Z

Pull request updated. Mostly changes from #2870

ide · 2015-09-20T01:02:11Z

ReactAndroid/src/main/java/com/facebook/react/modules/websocket/WebSocketModule.java

+
+    if (mWebSocketClient != null) {
+      try {
+        mWebSocketClient.close(1000, "");


why 1000?

nvm: can you add a comment or assign this to a variable/constant so it's documented?

ide · 2015-09-20T01:07:07Z

I really want to share more JS between iOS and Android. My thinking is that we should merge #2599 first, to get iOS's implementation even better, and then start moving code out of WebSocket.ios.js into WebSocketBase.js till only a few lines need to be specific to iOS or Android.

@mkonicek can you do a pass over the Java code style?

satya164 · 2015-09-20T01:41:14Z

@ide The status code 1000 means CLOSE_NORMAL.

I added the comments.

satya164 · 2015-09-20T01:43:13Z

@ide Also, the JavaScript code of both iOS and Android implementations are exactly the same. So when the pull request is merged, we can just copy/paste the code.

ide · 2015-09-20T01:57:33Z

@satya164 If the code is exactly the same then I'm proposing that we rename WebSocket.ios.js to WebSocket.js and delete the platform-specific files.

satya164 · 2015-09-20T02:01:01Z

@ide Okay, so I was looking at the implementations and noticed that the close method didn't accept any parameters as per the spec. I changed the Android code so that it now accepts a code and a reason as argument, and the code is no longer the same :(

I'll look at the pull request for iOS and see if I can do those changes in my pull request for Android also (after it is merged).

satya164 · 2015-09-22T13:50:30Z

Any updates on this?

satya164 · 2015-09-30T18:20:56Z

@foghina @ide Would love to get this merged!

vjeux · 2015-10-01T16:23:51Z

Libraries/WebSocket/WebSocket.android.js

+        this.onopen && this.onopen();
+      }),
+      RCTDeviceEventEmitter.addListener('websocketClosed', ev => {
+          if (ev.id !== id) {


nit: can you indent only 2 spaces

mkonicek · 2015-10-02T12:35:11Z

I'm haven't yet tried the shipit bot on PRs where the commits haven't been squashed. Let's see what happens :)

facebook-github-bot · 2015-10-02T12:36:40Z

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1661554830752790/int_phab to review.

foghina · 2015-10-02T12:48:39Z

@mkonicek probably this.

mkonicek · 2015-10-02T14:06:25Z

Import looks good, except it failed because Network.md doesn't exist in the fb codebase - it is github only. Can you please remove the Network.md from this PR? We can edit it directly on github once this lands. We should probably also only update the docs once this gets released (to npm and Maven Central) which is when people can start to use it.

mkonicek · 2015-10-02T14:07:28Z

Just send a separate PR for the docs please.

mkonicek · 2015-10-02T14:11:37Z

Actually let me do that.

mkonicek · 2015-10-02T14:14:36Z

Libraries/WebSocket/MessageEvent.js

+
+'use strict';
+
+var Event = require('Event');


Just curious - this is quite a generic name and could conflict with other code in the future. How about calling it WebSocketEvent?

satya164 · 2015-10-02T14:23:20Z

@mkonicek I made the same changes as 626b551

I had to move those into separate files since we had 2 different files for iOS and Android. I believe the pull request was to provide a proper Event class. If I change the name it will be no longer be the case.

It's generic enough though, and not tied to WebSockets in any ways. So it could be used for other events also.

But if you want, I could inline them to the WebSocket.js file, so it'll not conflict with other files.

mkonicek · 2015-10-02T16:34:54Z

Thanks for the explanation! I think Event is fine for now. The module system we use internally at fb uses global names so the websocket Event will be available anywhere using require('Event'), that's why I was suggesting require('WebSocketEvent').

mkonicek · 2015-10-02T16:36:46Z

I'll work on merging this internally now, need to make some changes to the Buck build scripts we use internally to build React Native.

mkonicek · 2015-10-02T19:59:19Z

Still working on merging, will continue on Monday. No need to make more changes to the PR, looks good now. I'll just inline the Event into MessageEvent as Event is going to conflict with a class of the same name we have on the web.

satya164 · 2015-10-02T20:06:27Z

@mkonicek Awesome. Thanks a lot :D

satya164 · 2015-10-06T09:24:04Z

@mkonicek Any updates on this?

mkonicek · 2015-10-06T17:48:14Z

I had to update the Java files a bit, merging now..

mkonicek · 2015-10-06T23:54:33Z

This got delayed because it's failing some internal iOS tests. Looking.

mkonicek · 2015-10-07T01:02:50Z

Will have to rebase once the fb internal tests are fixed and commit then. Hang tight! Thanks for the hard work, I literally just need to get this into the internal fb repo now :)

satya164 · 2015-10-07T08:37:14Z

@mkonicek Thanks a lot for keeping me updated. Much appreciated :D

mkonicek · 2015-10-07T15:29:27Z

No problem ☺

vjeux · 2015-10-07T15:38:13Z

Yay!

satya164 · 2015-10-07T16:36:28Z

Awesome!

mkonicek · 2015-10-07T17:14:24Z

@satya164 The PR had to be split into two commits because of a technical detail of how the sync with the internal fb codebase works: Network.md lives on github only, all the other files also live in the fb codebase. Currently we have to commit these two types of changes separately.

I committed the docs for you which saves you a bit of work but makes the comit blame to me: c819c55

Let me know if you'd like to split the PR into two yourself next time and keep attribution that way.

mkonicek · 2015-10-07T17:15:04Z

Long term we should probably have a way to auto-split the PR, don't have enough context on github APIs yet to estimate how much work that would be.

satya164 · 2015-10-07T17:17:51Z

@mkonicek No problem. Yeah, sure, I can do the work next time :D

This was a really small commit though, so it's alright :)

@vjeux

Summary: The JavaScript code for Android is same as the iOS counterpart, I just added few new lines and used arrow functions instead of binding `this`. Closes facebook#2839 Reviewed By: @svcscm, @vjeux Differential Revision: D2498703 Pulled By: @mkonicek fb-gh-sync-id: 3fe958dd5af0efba00df07515f8e33b5d87eb05b

@vjeux

Summary: The JavaScript code for Android is same as the iOS counterpart, I just added few new lines and used arrow functions instead of binding `this`. Closes facebook#2839 Reviewed By: @svcscm, @vjeux Differential Revision: D2498703 Pulled By: @mkonicek fb-gh-sync-id: 3fe958dd5af0efba00df07515f8e33b5d87eb05b

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 18, 2015

foghina reviewed Sep 18, 2015
View reviewed changes

foghina added GH Review: needs-revision and removed GH Review: review-needed labels Sep 18, 2015

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Sep 18, 2015

mkonicek added the GH Review: needs-revision label Sep 18, 2015

mkonicek removed the GH Review: review-needed label Sep 18, 2015

mkonicek added the Android label Sep 19, 2015

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Sep 20, 2015

ide reviewed Sep 20, 2015
View reviewed changes

satya164 mentioned this pull request Sep 27, 2015

Implement WebSocket module for Android with EventTarget interface #3079

Closed

satya164 mentioned this pull request Oct 1, 2015

WebSocket API Polyfill #619

Closed

vjeux reviewed Oct 1, 2015
View reviewed changes

mkonicek reviewed Oct 2, 2015
View reviewed changes

ghost closed this in f4857a6 Oct 7, 2015

mkonicek pushed a commit that referenced this pull request Oct 7, 2015

Add @satya164's WebSocket docs from #2839

c819c55

puf mentioned this pull request Oct 23, 2015

Add support for react-native-Android FirebaseExtended/reactfire#72

Closed

MattFoley pushed a commit to skillz/react-native that referenced this pull request Nov 9, 2015

Add @satya164's WebSocket docs from facebook#2839

3860b09

Crash-- pushed a commit to Crash--/react-native that referenced this pull request Dec 24, 2015

Add @satya164's WebSocket docs from facebook#2839

95bf05a

This pull request was closed.

Implement WebSocket module for Android. Fixes #2837 #2839

Implement WebSocket module for Android. Fixes #2837 #2839

Conversation

satya164 commented Sep 18, 2015

foghina Sep 18, 2015

Choose a reason for hiding this comment

satya164 Sep 18, 2015

Choose a reason for hiding this comment

ide Sep 18, 2015

Choose a reason for hiding this comment

foghina commented Sep 18, 2015

foghina commented Sep 18, 2015

satya164 commented Sep 18, 2015

satya164 commented Sep 18, 2015

mkonicek commented Sep 18, 2015

satya164 commented Sep 18, 2015

satya164 commented Sep 20, 2015

ide Sep 20, 2015

Choose a reason for hiding this comment

ide commented Sep 20, 2015

satya164 commented Sep 20, 2015

satya164 commented Sep 20, 2015

ide commented Sep 20, 2015

satya164 commented Sep 20, 2015

satya164 commented Sep 22, 2015

satya164 commented Sep 30, 2015

vjeux Oct 1, 2015

Choose a reason for hiding this comment

mkonicek commented Oct 2, 2015

facebook-github-bot commented Oct 2, 2015

foghina commented Oct 2, 2015

mkonicek commented Oct 2, 2015

mkonicek commented Oct 2, 2015

mkonicek commented Oct 2, 2015

mkonicek Oct 2, 2015

Choose a reason for hiding this comment

satya164 commented Oct 2, 2015

mkonicek commented Oct 2, 2015

mkonicek commented Oct 2, 2015

mkonicek commented Oct 2, 2015

satya164 commented Oct 2, 2015

satya164 commented Oct 6, 2015

mkonicek commented Oct 6, 2015

mkonicek commented Oct 6, 2015

mkonicek commented Oct 7, 2015

satya164 commented Oct 7, 2015

mkonicek commented Oct 7, 2015

vjeux commented Oct 7, 2015

satya164 commented Oct 7, 2015

mkonicek commented Oct 7, 2015

mkonicek commented Oct 7, 2015

satya164 commented Oct 7, 2015