Add support for sending binary data in websockets #6961

philikon · 2016-04-13T18:12:46Z

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

facebook-github-bot · 2016-04-13T18:13:19Z

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

philikon · 2016-04-13T18:14:27Z

Note: this PR depends on (and for now includes) #6889 and #6868. So reviewers would want to focus on the last commit only. Once these PRs get merged, I'll rebase this PR.

(I miss phabricator :p)

eslint-bot · 2016-04-13T18:18:13Z

Libraries/WebSocket/WebSocket.js

@@ -11,114 +11,174 @@
 */
 'use strict';

-var RCTDeviceEventEmitter = require('RCTDeviceEventEmitter');
-var RCTWebSocketModule = require('NativeModules').WebSocketModule;


no-undef: 'ArrayBufferView' is not defined.

facebook-github-bot · 2016-04-13T18:52:14Z

@philikon updated the pull request.

eslint-bot · 2016-04-13T18:56:31Z

Libraries/WebSocket/WebSocket.js

-var WebSocketEvent = require('WebSocketEvent');
+const RCTDeviceEventEmitter = require('RCTDeviceEventEmitter');
+const RCTWebSocketModule = require('NativeModules').WebSocketModule;
+const Platform = require('Platform');


no-undef: 'ArrayBufferView' is not defined.

facebook-github-bot · 2016-04-13T19:03:16Z

@philikon updated the pull request.

philikon · 2016-04-14T19:14:08Z

Not sure what's up with the Travis failure. I've had a few other Travis builds fail last night, so perhaps Travis had some problems. Either way, I ran the e2e test locally and it passed. Maybe somebody with the right permission bits could kick Travis and restart the build.

mkonicek · 2016-04-15T15:31:15Z

Wow thanks for adding the WebSocketExample and a server!

@satya164 Do the changes in WebSocket.js look good to you?

mkonicek · 2016-04-15T15:32:59Z

This is quite a big rewrite - could you please also add sending strings on both Android and iOS to your test plan to make sure it doesn't break anything?

There are no breaking API changes, right?

mkonicek · 2016-04-15T15:33:32Z

Travis tests passed now, they can be a bit flaky (but much more stable than they used to be).

philikon · 2016-04-15T17:23:54Z

This is quite a big rewrite - could you please also add sending strings on both Android and iOS to your test plan to make sure it doesn't break anything?

Please see my comment above. Most of the changes in this PR are from being based on #6889. The actual changes here are only in the last commit and I will rebase once all the dependent PRs have been merged. But with review times of O(1 week), I just wanted to move faster.

philikon · 2016-04-19T00:22:09Z

@mkonicek @christopherdro now that the other PRs have landed, I have rebased this PR here. It is ready for review now. Thanks!

facebook-github-bot · 2016-04-19T00:22:16Z

@philikon updated the pull request.

eslint-bot · 2016-04-19T00:27:45Z

Libraries/WebSocket/WebSocket.js

+    // Maintain iOS 7 compatibility which doesn't have JS typed arrays.
+    if (typeof ArrayBuffer !== 'undefined' &&
+        typeof Uint8Array !== 'undefined') {
+      // $FlowIssue #1658


Error suppressing comment Unused suppression

mkonicek · 2016-04-19T11:15:50Z

@philikon Is this one good to go? Should I merge it? Please check the eslint warnings.

philikon · 2016-04-19T17:31:10Z

Ah, got stung by the Flow 0.23 upgrade in the rebase. Will fix tests.

facebook-github-bot · 2016-04-19T17:38:20Z

@philikon updated the pull request.

philikon · 2016-04-19T17:41:07Z

@mkonicek Addressed your review comment and hopefully fixed Flow. Should be good to go now once CI comes back green.

zxcpoiu · 2016-04-19T18:08:29Z

@philikon nice job!

BTW, which version of base64-js module is required?

currently it is 0.0.8, and in #6327 they need to upgrade to 1.0.4 but have to be in sperated PR.

please take a look at #6937 and confirm.

thanks!

philikon · 2016-04-19T20:03:02Z

currently it is 0.0.8, and in #6327 they need to upgrade

They didn't need to upgrade. They just wanted to. I confirmed that it works without upgrading base64-js.

We/someone can upgrade base64-js separately if they want. Edit: I see you already did in #6937. Thanks!

mkonicek · 2016-04-20T14:57:12Z

@facebook-github-bot shipit

facebook-github-bot · 2016-04-20T15:03:59Z

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

philikon · 2016-04-20T18:04:39Z

\o/

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 13, 2016

eslint-bot reviewed Apr 13, 2016
View reviewed changes

philikon force-pushed the ws_send_binary branch from c85386f to 936d16b Compare April 13, 2016 18:51

eslint-bot reviewed Apr 13, 2016
View reviewed changes

philikon force-pushed the ws_send_binary branch from 936d16b to c5d9469 Compare April 13, 2016 19:02

philikon mentioned this pull request Apr 14, 2016

Add support for sending binary data in websockets. #6327

Closed

mkonicek mentioned this pull request Apr 15, 2016

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

Closed

philikon force-pushed the ws_send_binary branch from c5d9469 to a6cca78 Compare April 19, 2016 00:21

eslint-bot reviewed Apr 19, 2016
View reviewed changes

philikon force-pushed the ws_send_binary branch from a6cca78 to 0231bb5 Compare April 19, 2016 17:37

Add support for sending binary data in websockets

a3c1835

philikon force-pushed the ws_send_binary branch from 0231bb5 to a3c1835 Compare April 19, 2016 17:40

zxcpoiu mentioned this pull request Apr 19, 2016

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

Closed

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Apr 20, 2016

ghost closed this in ed930b4 Apr 20, 2016

philikon deleted the ws_send_binary branch April 20, 2016 18:04

zxcpoiu mentioned this pull request Jun 8, 2016

Android WebSockets: include cookie in request #7989

Closed

robclouth mentioned this pull request Oct 13, 2016

ndarray support gre/gl-react-native-v2#14

Closed

This pull request was closed.

Add support for sending binary data in websockets #6961

Add support for sending binary data in websockets #6961

Uh oh!

Conversation

philikon commented Apr 13, 2016

Uh oh!

facebook-github-bot commented Apr 13, 2016

Uh oh!

philikon commented Apr 13, 2016

Uh oh!

eslint-bot Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 13, 2016

Uh oh!

eslint-bot Apr 13, 2016

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 13, 2016

Uh oh!

philikon commented Apr 14, 2016

Uh oh!

mkonicek commented Apr 15, 2016

Uh oh!

mkonicek commented Apr 15, 2016

Uh oh!

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

Uh oh!

Uh oh!

philikon commented Apr 15, 2016

Uh oh!

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

Uh oh!

Uh oh!

facebook-github-bot commented Apr 19, 2016

Uh oh!

eslint-bot Apr 19, 2016

Choose a reason for hiding this comment

Uh oh!

mkonicek commented Apr 19, 2016

Uh oh!

philikon commented Apr 19, 2016

Uh oh!

facebook-github-bot commented Apr 19, 2016

Uh oh!

philikon commented Apr 19, 2016

Uh oh!

zxcpoiu commented Apr 19, 2016

Uh oh!

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

Uh oh!

Uh oh!

mkonicek commented Apr 20, 2016

Uh oh!

facebook-github-bot commented Apr 20, 2016

Uh oh!

philikon commented Apr 20, 2016

Uh oh!

Uh oh!

mkonicek commented Apr 15, 2016 •

edited

Loading

philikon commented Apr 19, 2016 •

edited

Loading

philikon commented Apr 19, 2016 •

edited

Loading