Add support for sending binary data in websockets by philikon · Pull Request #6961 · react/react-native

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

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

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

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 react#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 react#6889) and test sending binary data on both iOS and Android Closesreact#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

Comment threadLibraries/WebSocket/WebSocket.js Outdated
eslint-botApr 13, 2016
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-undef: 'ArrayBufferView' is not defined.

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

eslint-bot reviewed Apr 13, 2016
View reviewed changes

Comment threadLibraries/WebSocket/WebSocket.js Outdated
eslint-botApr 13, 2016
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-undef: 'ArrayBufferView' is not defined.

philikon force-pushed the ws_send_binary branch from 936d16b to c5d9469Compare 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 a6cca78Compare April 19, 2016 00:21

eslint-bot reviewed Apr 19, 2016
View reviewed changes

Comment threadLibraries/WebSocket/WebSocket.js Outdated
eslint-botApr 19, 2016
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error suppressing comment Unused suppression

philikon force-pushed the ws_send_binary branch from a6cca78 to 0231bb5Compare 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 a3c1835Compare 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 ed930b4Apr 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.

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

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 13, 2016

Uh oh!

eslint-botApr 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-botApr 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mkonicek commented Apr 15, 2016•
edited
Loading

philikon commented Apr 19, 2016•
edited
Loading

philikon commented Apr 19, 2016•
edited
Loading