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

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@jeremyong updated the pull request.

It's not a very efficient way to send binary data by Base64. You'll need to store a large chunk of data in memory, duplicated across the Native and JS threads, as well as transfer a large amount of data over the bridge, which can slow everything down.

In addition, ArrayBuffers are not supported on iOS7. Since Facebook still needs to support iOS7, we can't merge this.

Closing this PR, please send another PR of you manage to find a more efficient way of doing this.

I'm not sure I agree with the conclusion. The bridge marshalls data over as JSON right? I don't see how this can be done without a heavy overhaul of the bridge.

In the meantime, websocket only handles TEXT which is being copied over regardless! Whether it's binary or text makes no difference but because RN does not support binary, developers are forced to base64 encode binary and send it over the network which is far worse than an in memory copy.

With respect to ArrayBuffers, creating a different overload for interpreting a string as binary works easily enough. But as it stands, I believe the current WS code/bridge to be completely unusable. There is an entire frame opcode type that is unavailable for use, and I would prefer full functionality with well documented caveats over no functionality at all. The workarounds in this case are strictly worse.

edit:

My final question I suppose is, if you have an performance problem with how this is working, why isn't literally everything else that gets marshalled back and forth across the RN bridge a problem too?

Another question, if the stipulation is that you don't want to copy across the bridge, do you at least have another suggestion for going about this? If a copy isn't made, than the implementer needs to make some guarantee that the JS execution thread won't attempt to modify the memory. What mechanism can be used to prevent this?

I totally agree with @jeremyong. I would prefer a working solution that might not be the most efficient one over a solution that do not work at all. Is there another way of solving this issue?

@jeremyong

I'm not sure I agree with the conclusion. The bridge marshalls data over as JSON right? I don't see how this can be done without a heavy overhaul of the bridge.

I think the bridge needs to support streaming data, or have a new way of streaming data. For which we don't have a solution yet.

In the meantime, websocket only handles TEXT which is being copied over regardless! Whether it's binary or text makes no difference but because RN does not support binary, developers are forced to base64 encode binary and send it over the network which is far worse than an in memory copy.

There's a big difference between text and binary files. Binary files are mostly significantly larger than text files. base64 encoding it means even larger size.

With respect to ArrayBuffers, creating a different overload for interpreting a string as binary works easily enough. But as it stands, I believe the current WS code/bridge to be completely unusable. There is an entire frame opcode type that is unavailable for use, and I would prefer full functionality with well documented caveats over no functionality at all. The workarounds in this case are strictly worse.

If you can provide a sane fallback for iOS 7 with ArrayBuffer for new versions, that's acceptable too.

My final question I suppose is, if you have an performance problem with how this is working, why isn't literally everything else that gets marshalled back and forth across the RN bridge a problem too?

Everything which goes across the bridge is very small amount of data. So its not a problem. It's a problem if you try sending a 5MB of data across the bridge. The bridge is not designed for that. And I'm afraid I don't have a solution for this yet.

Another question, if the stipulation is that you don't want to copy across the bridge, do you at least have another suggestion for going about this? If a copy isn't made, than the implementer needs to make some guarantee that the JS execution thread won't attempt to modify the memory. What mechanism can be used to prevent this?

One thing I can think of is to have a proprietary extension to WebSocket like we do in XMLHttpRequest. For example, we allow sending a file through ajax by specifying a file URI on JS side. The native side handles reading the file and sending it to remote.

cc @dmmiller @mkonicek @nicklockwood

I totally agree with @jeremyong. I would prefer a working solution that might not be the most efficient one over a solution that do not work at all. Is there another way of solving this issue?

It's not just 'might not be the most efficient', it's not at all efficient. If we add this, people will start abusing it.

There's a big difference between text and binary files. Binary files are mostly significantly larger than text files. base64 encoding it means even larger size.

Binary data is binary data. 8 bytes of binary data and 8 bytes of text data is 8 bytes. I don't understand your reasoning. Sending something across the bridge as binary data doesn't mean I'm reading MP3s from the disk and sending it! Many of us use binary protocols to talk over the web. Trust me, I'm very familiar with the overhead of base64 encoding and honestly it hurts me that I even have to do this.

If you can provide a sane fallback for iOS 7 with ArrayBuffer for new versions, that's acceptable too.

If I address the arrayBuffer concern, which isn't hard, it doesn't seem like you'll accept regardless until we get the performance thing hashed out.

One thing I can think of is to have a proprietary extension to WebSocket like we do in XMLHttpRequest. For example, we allow sending a file through ajax by specifying a file URI on JS side. The native side handles reading the file and sending it to remote.

Not everyone is trying to send files. If the bridge supports streaming or chunked transfer (or asynchronous transfer on a different thread in the future) I'm all for that, but NO binary support until then? That's just madness and you're going to have a ton of people using unofficial releases of RN until that happens.

It's not just 'might not be the most efficient', it's not at all efficient. If we add this, people will start abusing it.

This is how features never happen. In every codebase. edit: as an addendum to this comment, I should point out that my philosophy is that eventually (not necessarily now) someone will try to marshall an obscene amount of data over the bridge, and when that point comes, effort can be made to search for a more optimal path. Ironically, that more optimal path may be slower for small packets of binary data, which I honestly believe will be the more common use case (for users who use websockets to transfer data serialized by protobuf, thrift, avro, capnproto, or any other binary format).

Some thoughts:

There are two scenarios for needing to transfer binary data over a web socket or XHR:

1. The binary data will be needed on the native side, e.g. to display an image, store in a file, or upload to some other destination.
2. The binary data will be needed on the JS side, which will process it in some way (e.g. for an image it might need to do some kind of per-pixel processing).

We desperately want to avoid people trying to solve (1) by base64-encoding the data, because that means encoding and decoding the data at least twice - once to transfer it to JS, and then again to send it back to native. This is horrible, and there are usually much better workarounds. For example, for images we store them on the native side and just send URIs to JS. This might work for arbitrary binary data too, but there's a problem with lifecycle (how do you know when JS has discarded the URI so you can release the data on the native side),

Scenario (2) is a more reasonable justification for using base64, but it's probably a bad idea as well because JS is orders of magnitude slower than native code. If you need to do custom data manipulation, better to write a native plugin.

So basically, if you have a use case for this, it's better to describe precisely what you're trying to do and then we can discuss if there's a domain-specific solution that can solve it efficiently. We'd prefer that to introducing a general solution that people will then go on and use for cases where it yields terrible performance (and then complain about how much RN perf sucks, etc.).

@nicklockwood I've already said it multiple times. Binary protocols.

https://github.com/google/protobuf
https://thrift.apache.org/

etc. Websockets are just a framing protocol that exists on top of TCP, and there's no reason to restrict that what is transfered over them must be UTF-8 encoded text. I can make a trivially small protobuf message that fails to send in the current implementation because of that restriction.

Binary does not mean just images, videos, audio, etc. If a programmer decides that a byte with the 2 highest bits set and the lowest bit set means something, he or she should feel free to do so, and interpret it that way both in JS and on the server. With this interface anyway, I'd be really surprised if someone thought sending whole high res images over websocket was a good idea. I think there is a danger in working too hard to protect the developer, who many very well know what he or she is doing.

The main thing I'm not sure I know how to do, is transfer arbitrary binary data over the bridge if I don't have ArrayBuffers to work with, and this is what I would appreciate feedback on. What happens to byte sequences that get transferred that isn't valid UTF-8 if it's encapsulated in JSON? Do I need the ArrayBuffer here? If not, I can just make a separate function for sending binary as opposed to making it an overload and remove the encoding/decoding step.

We'd prefer that to introducing a general solution that people will then go on and use for cases where it yields terrible performance (and then complain about how much RN perf sucks, etc.).

FWIW, I never take much stock in what those people say. People will always find a way to abuse your code, and I really doubt there's much use in trying to protect yourself from it (for example, people right now might already be base64 encoding binary in JS and sending it as text, only to reverse on server-side instead. they might be doing this for 500 gb movies for all I know). Having a great framework like RN is no excuse for not knowing how it works or how it should be used. Frankly, I'd prefer if people were trained to think critically about the things they read instead of just being blown by the wind every time a juicy headline showed up.

OK, after an impassioned plea from @dmmiller, I think maybe we can try landing this and see what happens, as it does provide a useful feature that we don't (currently) have a better solution for.

The remaining blocker is iOS 7 support. We don't actually need this to work on iOS 7, since we aren't using it internally, but we do need it not to crash. I'm guessing it's straightforward to do a runtime check that ArrayBuffer is available, and display an error if not?

Ah great thanks

The main thing I'm not sure I know how to do, is transfer arbitrary binary data over the bridge if I don't have ArrayBuffers to work with, and this is what I would appreciate feedback on. What happens to byte sequences that get transferred that isn't valid UTF-8 if it's encapsulated in JSON? Do I need the ArrayBuffer here? If not, I can just make a separate function for sending binary as opposed to making it an overload and remove the encoding/decoding step.

(quoting myself here) I'll see if I can experiment a little today and see if arbitrary binary sequences get marshaled over untampered with. In which case I can avoid the Base64 step entirely. If this was known in advance, I would appreciate any advice.

If you do find a way to transfer raw bytes (including invalid UTF-8) over the the bridge, please add unit tests so that someone working on the bridge doesn't assume that they're working purely with valid UTF-8 and break things?

Also, RN Android's C++ has freaked out with invalid UTF-8 (emoji even, though that's been fixed) so be sure to check that too.

Re: iOS7, can you explicitly throw an error as early as possible that says something like "Sending ArrayBuffers is not supported in this environment because it does not implement typed arrays."?

I finally got a chance to revisit this just now. If ArrayBuffers is a no-go, it looks like this won't work after all. The current websocket implementation for javascript blindly decodes the method as a UTF8 string, ignoring the frame opcode which breaks any attempt from the server to send binary data.

Given that the project's current direction is to not support ArrayBuffers (to hold on to iOS 7 which has ~3% market penetration based on recently openly published mixpanel data), I've decided to just move us forward internally for the time being and extend the bridge to have proper support for ArrayBuffers so I won't have to do this.

Thanks for the comments and feedback thus far.

Oh hahaha I just found #4483 so apparently, there already is a case where we base64 encode messages received from the server to marshall it over the bridge on iOS. I panicked because I realized none of my reads were guaranteed to work either.

Based on this discussion are you guys going to revert that PR then? It's kind of odd that the proper handling is a single direction (server to client) for a single platform (ios).

On ArrayBuffers -- @nicklockwood said they're fine to use despite not being supported on iOS 7 because sending binary data over Web Sockets isn't something that most apps need to do, and people who want to rely on this feature can target iOS 8+. The important thing is that using ArrayBuffers should not crash an app on iOS 7 if the app doesn't use the feature.

I suspect you will need to convert ArrayBuffers -> base 64 -> JSON. Maybe there's another way.

Based on this discussion are you guys going to revert that PR then? It's kind of odd that the proper handling is a single direction (server to client) for a single platform (ios).

This is a bit meta, but given the scope of RN and the number of people working on it, expect there to be inconsistencies.

It might be a bit meta but there are people on this thread that were reviewers of the other.

We aren't going to revert the other PR - I'd forgotten that we'd accepted it, but that's what happens when these conversations drag on over months ¯_(ツ)_/¯. The reason we only have one-way binary comms is that that's all that the original PR author needed.

Base64 is the only safe way to encode arbitrary binary data over JSON (that I know of). We can certainly explore alternative mechanisms that use JSC to bypass JSON altogether, but we need a mechanism that works over JSON as a starting point because of the Chrome debugger.

And yes, it's fine to use ArrayBuffer on iOS 8+, we don't want to hold the platform back because of iOS 7, but we need the code to be written in such a way that it doesn't blow up on a version of JSC that doesn't support it. There are a bunch of ways to do runtime feature detection in JS - see how the fetch polyfill handles Blob support, for example.

@jeremyong

I appreciate all the works that you and @dewe have done, really.

But things got to moving on, how about we implement checks for ios7 as @ide said and post a diff patch here then you can use it to update this PR?

or make a PR on your repo?

Yeah, now I've browsed through this thread. As far as I can see, updates needed: the IOS7 if (typeof Uint8Array === 'undefined') check, and replacing the Java7 catch with Java6. Anything else?

Edit: split out package.json update to a separate PR. It's not really needed for this PR.

Thanks for looking @dewe!

Here's a patch file that (hopefully) can be applied to @jeremyong's repo.

patchfile.txt

Thanks @dewe

I made a PR here PlexChat/react-native#1
correct me if anything I did wrong.

@jeremyong
Can you help to merge that when you're free?

Hey folks, I'm happy to take over this PR and get it to land, with iOS 7 support and all. Unless somebody wants to beat me to it?

@satya164 would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

Actually, #6961 is probably what should be reviewed at this point.

Let's merge https://github.com/facebook/react-native/pull/6961based on @philikon's comment and the comments above.