Clean up and simplify WebSocket implementation on the JS side #6889

philikon · 2016-04-08T23:58:08Z

Get rid of no longer necessary WebSocket.js v WebSocketBase.js split
Use EventTarget(list, of, events) as base class to auto-generate oneventname getters/setters that get invoked along with other event handlers
Type annotation any considered harmful, especially when we can easily spell out the actual type
Throw in some const goodness for free

Test Plan: Launch UIExplorer example app, supplied websocket_test_server script, and try different combinations of sending and receiving text and binary data on both iOS and Android.

facebook-github-bot · 2016-04-08T23:58:57Z

By analyzing the blame information on this pull request, we identified @hharnisc, @satya164 and @aprock to be potential reviewers.

facebook-github-bot · 2016-04-09T02:31:20Z

@philikon updated the pull request.

facebook-github-bot · 2016-04-11T03:08:14Z

@philikon updated the pull request.

christopherdro · 2016-04-12T05:33:38Z

@philikon Nice! I'll should be able to pull this into the current project I'm working on to test.

I've noticed you've been working on a couple stuff with Websockets lately and was just curious how your testing these new features and if you so on both iOS and Android?

Thanks again!

satya164 · 2016-04-12T05:38:20Z

Libraries/WebSocket/WebSocket.js


-  cancelConnectionImpl(): void {
+    this.readyState = this.CLOSING;
    this._closeWebSocket(this._socketId);


The spec allows to specify a reason, and the Android implementation supports it also. Can you add it?

Sure. Can I do that in a follow-up? It'd be a new feature. I wanted to keep this PR here purely mechanical.

@philikon Not a new feature really, since closeConnectionImpl already has it, and it was left out in the close method. But I'm fine with either.

This is the original close() method:

close(): void { if (this.readyState === this.CLOSING || this.readyState === this.CLOSED) { return; } if (this.readyState === this.CONNECTING) { this.cancelConnectionImpl(); } this.readyState = this.CLOSING; this.closeConnectionImpl(); }

So, yes, while closeConnectionImpl() takes those parameters, close() never passed them on. Happy to add it, though.

satya164 · 2016-04-12T05:46:17Z

Can you provide a test plan?

satya164 · 2016-04-12T05:47:22Z

Libraries/WebSocket/WebSocket.js

+        event = new WebSocketEvent('close');
+        event.message = ev.message;
+        this.dispatchEvent(event);
+


nit: unnecessary new line

Sure, I can remove the newlines. I just added them to separate the repetitive stanzas for easier readability.

Actually, I just removed the newlines and I find the method so much harder to parse now. What's the reasoning here for declaring the newline "unnecessary"?

@philikon I prefer a lot of new lines in code, but that's not how React Native's authors prefer it.

Seems like we're in agreement then. :)

I think I can count myself as one of React Native's authors, so I'll just leave them in there until somebody complaints.

satya164 · 2016-04-12T05:51:48Z

There's some inconsistency regarding where you pass the id and where you use this._socketId. For example, _unregisterEvents uses this._socketId and the id is passed to _closeWebSocket. Personally I like to pass things as an argument even if it's on the instance. Helps in refactoring since those methods stay pure, and also decreases the number of keypresses, (id vs this._socketId).

philikon · 2016-04-12T20:48:14Z

There's some inconsistency regarding where you pass the id and where you use this._socketId. For example, _unregisterEvents uses this._socketId and the id is passed to _closeWebSocket.

Thanks for pointing that out. I'll fix that.

Personally I like to pass things as an argument even if it's on the instance. Helps in refactoring since those methods stay pure, and also decreases the number of keypresses, (id vs this._socketId).

I don't think the number of keypresses should be a measure for how we structure code. That kind of metric leads to overly short variable names that are no longer descriptive. Plus, I haven't seen your preferred style used anywhere else in React Native, in particular not in XMLHttpRequest, which is another similar web API that's we implemented for RN. Consistency within the project is what I'm mostly after, because it inspires 3rd party code. (Case in point: react-native-webrtc used the WebSocket implementation in RN as inspiration, which means it cargo-culted all of the unnecessary indirections etc. without questioning it.)

satya164 · 2016-04-12T21:10:43Z

I don't think the number of keypresses should be a measure for how we structure code. That kind of metric leads to overly short variable names that are no longer descriptive.

That was not the measure I used, my point was that it makes easier to refactor things which I said above, since making it a pure function decreases the coupling of the method to the class. I think this pattern is better than tightly coupled class methods. This incidentally also helps in decreasing keypresses, which I noticed and mentioned. You could name it socketId instead if id is not obvious to you. I just mentioned my preference and the reasons why I prefer it. But it's fine if you disagree.

Plus, I haven't seen your preferred style used anywhere else in React Native, in particular not in XMLHttpRequest, which is another similar web API that's we implemented for RN. Consistency within the project is what I'm mostly after, because it inspires 3rd party code. (Case in point: react-native-webrtc used the WebSocket implementation in RN as inspiration, which means it cargo-culted all of the unnecessary indirections etc. without questioning it.)

I don't think there's any preferred style anywhere in the codebase. Looking at XMLHttpRequest, its written in a slightly different way, so it's not really applicable there.

- Get rid of no longer necessary WebSocket.js v WebSocketBase.js split - Use `EventTarget(list, of, events)` as base class to auto-generate `oneventname` getters/setters that get invoked along with other event handlers - Type annotation `any` considered harmful, especially when we can easily spell out the actual type - Throw in some `const` goodness for free

philikon · 2016-04-12T22:45:07Z

I've noticed you've been working on a couple stuff with Websockets lately

Yep, and more to come. I'm planning on picking up #6327 and getting it into shape to land.

and was just curious how your testing these new features and if you so on both iOS and Android?

Ah, yes. I have a little test script + app. I should just add those as a UIExplorer example!

facebook-github-bot · 2016-04-12T22:55:16Z

@philikon updated the pull request.

eslint-bot · 2016-04-12T23:02:46Z

Examples/UIExplorer/WebSocketExample.js

+  };
+
+  _sendText = () => {
+    this.state.socket.send(this.state.outgoingMessage);


call of method send Method cannot be called on possibly null value null

facebook-github-bot · 2016-04-13T00:56:20Z

@philikon updated the pull request.

mkonicek · 2016-04-15T15:37:03Z

This PR and #6961 both add the example, which one should be merged?

mkonicek · 2016-04-15T15:41:02Z

Thanks for doing this! The test plan looks good. Shall we #shipit ?

mkonicek · 2016-04-15T15:48:58Z

@facebook-github-bot shipit

facebook-github-bot · 2016-04-15T15:49:58Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

philikon · 2016-04-15T16:44:53Z

@mkonicek #6961 is just on top of my other PRs. Once they've landed, I will rebase #6961.

philikon · 2016-04-18T17:28:08Z

What's blocking this from getting merged at this point?

Summary:This brings the same functionality that's already present on iOS, introduced in #4483, to Android: convert binary payloads to base64 strings and send them to JS land that way, where they'll be turned into an ArrayBuffer. **Test Plan:** Used test server from #6889 (in `--binary` mode) to send some binary data to the Android UIExplorer example (also from #6889). Verified it's received correctly as `ArrayBuffer`. Closes #6868 Differential Revision: D3184797 Pulled By: mkonicek fb-gh-sync-id: e78c640c43b3e41a75ddba79acc04e5eaab5667d fbshipit-source-id: e78c640c43b3e41a75ddba79acc04e5eaab5667d

mkonicek · 2016-04-18T21:37:28Z

@philikon Failed landcastle with unrelated errors which is quite common :( Just hit 'Re-ship' for you and will keep an eye on the status.

philikon · 2016-04-19T00:20:13Z

Woo, thanks!

Summary:This is a reprise of #6327, but with iOS 7.0 compatibility and less `package.json` changes. **Test Plan:** Load WebSocketExample in UIExplorer app and start websocket test server script (both provided in #6889) and test sending binary data on both iOS and Android Closes #6961 Differential Revision: D3202022 Pulled By: mkonicek fb-gh-sync-id: 38843d0a9c0172971c5c70a5139ded04042b280a fbshipit-source-id: 38843d0a9c0172971c5c70a5139ded04042b280a

Summary:This brings the same functionality that's already present on iOS, introduced in facebook#4483, to Android: convert binary payloads to base64 strings and send them to JS land that way, where they'll be turned into an ArrayBuffer. **Test Plan:** Used test server from facebook#6889 (in `--binary` mode) to send some binary data to the Android UIExplorer example (also from facebook#6889). Verified it's received correctly as `ArrayBuffer`. Closes facebook#6868 Differential Revision: D3184797 Pulled By: mkonicek fb-gh-sync-id: e78c640c43b3e41a75ddba79acc04e5eaab5667d fbshipit-source-id: e78c640c43b3e41a75ddba79acc04e5eaab5667d

Summary:- Get rid of no longer necessary WebSocket.js v WebSocketBase.js split - Use `EventTarget(list, of, events)` as base class to auto-generate `oneventname` getters/setters that get invoked along with other event handlers - Type annotation `any` considered harmful, especially when we can easily spell out the actual type - Throw in some `const` goodness for free **Test Plan:** Launch UIExplorer example app, supplied `websocket_test_server` script, and try different combinations of sending and receiving text and binary data on both iOS and Android. Closes facebook#6889 Differential Revision: D3184835 Pulled By: mkonicek fb-gh-sync-id: f21707f4e97aa5a79847f5157e0a9f132a1a01cd fbshipit-source-id: f21707f4e97aa5a79847f5157e0a9f132a1a01cd

Summary:This is a reprise of facebook#6327, but with iOS 7.0 compatibility and less `package.json` changes. **Test Plan:** Load WebSocketExample in UIExplorer app and start websocket test server script (both provided in facebook#6889) and test sending binary data on both iOS and Android Closes facebook#6961 Differential Revision: D3202022 Pulled By: mkonicek fb-gh-sync-id: 38843d0a9c0172971c5c70a5139ded04042b280a fbshipit-source-id: 38843d0a9c0172971c5c70a5139ded04042b280a

Summary:This is a reprise of #6327, but with iOS 7.0 compatibility and less `package.json` changes. **Test Plan:** Load WebSocketExample in UIExplorer app and start websocket test server script (both provided in #6889) and test sending binary data on both iOS and Android Closes facebook/react-native#6961 Differential Revision: D3202022 Pulled By: mkonicek fb-gh-sync-id: 38843d0a9c0172971c5c70a5139ded04042b280a fbshipit-source-id: 38843d0a9c0172971c5c70a5139ded04042b280a

Summary:This brings the same functionality that's already present on iOS, introduced in facebook#4483, to Android: convert binary payloads to base64 strings and send them to JS land that way, where they'll be turned into an ArrayBuffer. **Test Plan:** Used test server from facebook#6889 (in `--binary` mode) to send some binary data to the Android UIExplorer example (also from facebook#6889). Verified it's received correctly as `ArrayBuffer`. Closes facebook#6868 Differential Revision: D3184797 Pulled By: mkonicek fb-gh-sync-id: e78c640c43b3e41a75ddba79acc04e5eaab5667d fbshipit-source-id: e78c640c43b3e41a75ddba79acc04e5eaab5667d

Summary:- Get rid of no longer necessary WebSocket.js v WebSocketBase.js split - Use `EventTarget(list, of, events)` as base class to auto-generate `oneventname` getters/setters that get invoked along with other event handlers - Type annotation `any` considered harmful, especially when we can easily spell out the actual type - Throw in some `const` goodness for free **Test Plan:** Launch UIExplorer example app, supplied `websocket_test_server` script, and try different combinations of sending and receiving text and binary data on both iOS and Android. Closes facebook#6889 Differential Revision: D3184835 Pulled By: mkonicek fb-gh-sync-id: f21707f4e97aa5a79847f5157e0a9f132a1a01cd fbshipit-source-id: f21707f4e97aa5a79847f5157e0a9f132a1a01cd

Summary:This is a reprise of facebook#6327, but with iOS 7.0 compatibility and less `package.json` changes. **Test Plan:** Load WebSocketExample in UIExplorer app and start websocket test server script (both provided in facebook#6889) and test sending binary data on both iOS and Android Closes facebook#6961 Differential Revision: D3202022 Pulled By: mkonicek fb-gh-sync-id: 38843d0a9c0172971c5c70a5139ded04042b280a fbshipit-source-id: 38843d0a9c0172971c5c70a5139ded04042b280a

Summary:This brings the same functionality that's already present on iOS, introduced in facebook#4483, to Android: convert binary payloads to base64 strings and send them to JS land that way, where they'll be turned into an ArrayBuffer. **Test Plan:** Used test server from facebook#6889 (in `--binary` mode) to send some binary data to the Android UIExplorer example (also from facebook#6889). Verified it's received correctly as `ArrayBuffer`. Closes facebook#6868 Differential Revision: D3184797 Pulled By: mkonicek fb-gh-sync-id: e78c640c43b3e41a75ddba79acc04e5eaab5667d fbshipit-source-id: e78c640c43b3e41a75ddba79acc04e5eaab5667d

Summary:- Get rid of no longer necessary WebSocket.js v WebSocketBase.js split - Use `EventTarget(list, of, events)` as base class to auto-generate `oneventname` getters/setters that get invoked along with other event handlers - Type annotation `any` considered harmful, especially when we can easily spell out the actual type - Throw in some `const` goodness for free **Test Plan:** Launch UIExplorer example app, supplied `websocket_test_server` script, and try different combinations of sending and receiving text and binary data on both iOS and Android. Closes facebook#6889 Differential Revision: D3184835 Pulled By: mkonicek fb-gh-sync-id: f21707f4e97aa5a79847f5157e0a9f132a1a01cd fbshipit-source-id: f21707f4e97aa5a79847f5157e0a9f132a1a01cd

Summary:This is a reprise of facebook#6327, but with iOS 7.0 compatibility and less `package.json` changes. **Test Plan:** Load WebSocketExample in UIExplorer app and start websocket test server script (both provided in facebook#6889) and test sending binary data on both iOS and Android Closes facebook#6961 Differential Revision: D3202022 Pulled By: mkonicek fb-gh-sync-id: 38843d0a9c0172971c5c70a5139ded04042b280a fbshipit-source-id: 38843d0a9c0172971c5c70a5139ded04042b280a

Summary: **Motivation** This is originally a follow up of PR #6327 by dewe and jeremyong And mkonicek indicated this should split into a separate PR. Now philikon takes over #6327 as #6961 **Test Plan:** According to #6961 and #6889 Load WebSocketExample in UIExplorer app and start websocket test server script and test sending binary data on both iOS and Android Closes #6937 Reviewed By: javache Differential Revision: D3669596 Pulled By: mkonicek fbshipit-source-id: 342e29eb34de882bcbd9f297aab71dd6bb236748

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 Apr 8, 2016

philikon mentioned this pull request Apr 8, 2016

What's with the Foobar v. FoobarImpl stuff? react-native-webrtc/react-native-webrtc#41

Closed

philikon force-pushed the ws_cleanup branch from 7ca857e to c9e29af Compare April 9, 2016 02:30

philikon force-pushed the ws_cleanup branch from c9e29af to 876b1d7 Compare April 11, 2016 03:07

satya164 reviewed Apr 12, 2016
View reviewed changes

philikon force-pushed the ws_cleanup branch from 876b1d7 to 74d2a25 Compare April 12, 2016 22:54

eslint-bot reviewed Apr 12, 2016
View reviewed changes

This was referenced Apr 13, 2016

[WebSocket][android][ios parity] Add support for receiving binary type data (ArrayBuffer) #6868

Closed

Add support for sending binary data in websockets #6961

Closed

facebook-github-bot added GH Review: accepted and removed GH Review: review-needed labels Apr 15, 2016

ghost closed this in ebb44d2 Apr 18, 2016

philikon deleted the ws_cleanup branch April 19, 2016 00:20

zxcpoiu mentioned this pull request Apr 20, 2016

update base64-js version. follow up #6961 #6937

Closed

zxcpoiu mentioned this pull request Jun 8, 2016

Android WebSockets: include cookie in request #7989

Closed

This pull request was closed.

Clean up and simplify WebSocket implementation on the JS side #6889

Clean up and simplify WebSocket implementation on the JS side #6889

Uh oh!

Conversation

philikon commented Apr 8, 2016

Uh oh!

facebook-github-bot commented Apr 8, 2016

Uh oh!

facebook-github-bot commented Apr 9, 2016

Uh oh!

facebook-github-bot commented Apr 11, 2016

Uh oh!

christopherdro commented Apr 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satya164 commented Apr 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satya164 commented Apr 12, 2016

Uh oh!

philikon commented Apr 12, 2016

Uh oh!

satya164 commented Apr 12, 2016

Uh oh!

philikon commented Apr 12, 2016

Uh oh!

facebook-github-bot commented Apr 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 13, 2016

Uh oh!

mkonicek commented Apr 15, 2016

Uh oh!

mkonicek commented Apr 15, 2016

Uh oh!

mkonicek commented Apr 15, 2016

Uh oh!

facebook-github-bot commented Apr 15, 2016

Uh oh!

philikon commented Apr 15, 2016

Uh oh!

philikon commented Apr 18, 2016

Uh oh!

mkonicek commented Apr 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philikon commented Apr 19, 2016

Uh oh!

Uh oh!

mkonicek commented Apr 18, 2016 •

edited

Loading