Add responseType as a concept to RCTNetworking, send binary data as base64 #8324

philikon · 2016-06-22T18:35:57Z

In preparation for Blob support (wherein binary XHR and WebSocket responses can be retained as native data blobs on the native side and JS receives a web-like opaque Blob object), this change makes RCTNetworking aware of the responseType that JS requests. A xhr.responseType of '' or 'text' translates to a native response type of 'text'. A xhr.responseType of arraybuffer translates to a native response type of base64, as we currently lack an API to transmit TypedArrays directly to JS. This is analogous to how the WebSocket module already works, and it's a lot more versatile and much less brittle than converting a JS string back to a TypedArray, which is what's currently going on.

Now that we don't always send text down to JS, JS consumers might still want to get progress updates about a binary download. This is what the 'progress' event is designed for, so this change also implements that. This change also follows the XHR spec with regards to xhr.response and xhr.responseText:

if the response type is 'text', xhr.responseText can be peeked at by the JS consumer. It will be updated periodically as the download progresses, so long as there's either an onreadystatechange or onprogress handler on the XHR.
if the response type is not 'text', xhr.responseText can't be accessed and xhr.response remains null until the response is fully received. 'progress' events containing response details (total bytes, downloaded so far) are dispatched if there's an onprogress handler.

Once Blobs are landed, xhr.responseType of 'blob' will correspond to the same native response type, which will cause RCTNetworking to only send a blob ID down to JS, which can then create a Blob object from that for consumers.

Test Plan: This change comes with extensive changes and expansions to the XHRExamples in UIExplorer. onprogress handler, onreadystatechange handler, and responseType = 'arraybuffer' can be switched on independently. I tested all combinations on both iOS and Android.

ghost · 2016-06-22T18:36:14Z

By analyzing the blame information on this pull request, we identified @davidaurelio and @nicklockwood to be potential reviewers.

davidaurelio · 2016-06-23T09:33:54Z

@philikon amazing work

cc @javache

javache · 2016-06-23T13:25:08Z

Libraries/Network/XMLHttpRequest.js

-      (args) =>  this.__didReceiveData(...args)
+      (args) => this.__didReceiveData(...args)
+    ));
+    this._subscriptions.push(RCTNetworking.addListener(


I'm not super familiar with all of this code: does this mean we always send all the incremental data chunks and the complete response when the request finishes? That seems wasteful.

A valid question! But rest assured, we definitely don't send data twice to JS.

davidaurelio · 2016-06-23T13:32:37Z

Edge case question: what happens if the requestType property is changed in-flight? That is legal until the request headers are received.

philikon · 2016-06-23T16:10:33Z

Edge case question: what happens if the requestType property is changed in-flight? That is legal until the request headers are received.

Ah, good point. I failed to address that, both in code and the PR comments. I know the spec says that it's legal until request headers are received, but how useful is that really? I think we should make it illegal to change after the request has been sent -- this PR here would fail horribly otherwise anyway. Supporting the narrow gap between request sent and headers received seems very little benefit for a whole lot of code complexity...

davidaurelio · 2016-06-23T16:39:41Z

Yeah, I agree that it’s not worth implementing this. Just throw, or print a warning, that should be enough

philikon · 2016-06-25T05:31:19Z

Yeah, I agree that it’s not worth implementing this. Just throw, or print a warning, that should be enough

Done.

Who else should we get to review this so we can get it landed?

philikon · 2016-06-27T23:30:13Z

Ping?

mkonicek · 2016-06-28T07:45:44Z

@davidaurelio By assigning the PR it will show up in Pieter's diff queue - more visibility for this PR from our core contributor Philipp.

javache · 2016-06-28T23:42:04Z

Libraries/Network/RCTNetworking.android.js

+    responseType: 'text' | 'base64',
+    incrementalUpdates: boolean,
+    timeout: number,
+    callback: (requestId: number) => any


javache · 2016-06-28T23:58:50Z

Objective-C and JS changes LGTM, maybe @andreicoman11 or @foghina can help out with the native Android changes.

davidaurelio · 2016-06-29T13:41:58Z

lgtm, let’s wait for @foghina or @andreicoman11

philikon · 2016-06-30T01:29:15Z

Thanks for the review, @javache! Addressed your comments.

foghina · 2016-06-30T10:50:37Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

+        public Response intercept(Interceptor.Chain chain) throws IOException {
+          Response originalResponse = chain.proceed(chain.request());
+          ProgressResponseBody responseBody = new ProgressResponseBody(
+            originalResponse.body(), new ProgressListener() {


Formatting around here seems wrong. Should either be:

... = new ProgressResponseBody(..., ... { long last = ...

if it doesn't exceed 100 chars width, or

... = new ProgressResponseBody( ..., ... { long last = ...

Ok, I can fix that.

foghina · 2016-06-30T11:18:44Z

My comments are mostly questions because apparently our networking module is now a commentless monstrosity and I don't get how things work. Code looks good though. Please make sure to test uploading with progress as well (I think that was supported?).

philikon · 2016-06-30T22:02:00Z

My comments are mostly questions because apparently our networking module is now a commentless monstrosity and I don't get how things work.

:(

I can try and help document things some time.

Please make sure to test uploading with progress as well (I think that was supported?).

Yep. Still works. UIExplorer examples FTW :)

…ase64 In preparation for Blob support (wherein binary XHR and WebSocket responses can be retained as native data blobs on the native side and JS receives a web-like opaque Blob object), this change makes RCTNetworking aware of the responseType that JS requests. A `xhr.responseType` of `''` or `'text'` translates to a native response type of `'text'`. A `xhr.responseType` of `arraybuffer` translates to a native response type of `base64`, as we currently lack an API to transmit TypedArrays directly to JS. This is analogous to how the WebSocket module already works, and it's a lot more versatile and much less brittle than converting a JS *string* back to a TypedArray, which is what's currently going on. Now that we don't always send text down to JS, JS consumers might still want to get progress updates about a binary download. This is what the `'progress'` event is designed for, so this change also implements that. This change also follows the XHR spec with regards to `xhr.response` and `xhr.responseText`: - if the response type is `'text'`, `xhr.responseText` can be peeked at by the JS consumer. It will be updated periodically as the download progresses, so long as there's either an `onreadystatechange` or `onprogress` handler on the XHR. - if the response type is not `'text'`, `xhr.responseText` can't be access and `xhr.response` remains `null` until the response is fully received. `'progress'` events containing response details (total bytes, downloaded so far) are dispatched if there's an `onprogress` handler. Once Blobs are landed, `xhr.responseType` of `'blob'` will correspond to the same native response type, which will cause RCTNetworking to only send a blob ID down to JS, which can then create a `Blob` object from that for consumers. **Test Plan:** This change comes with extensive changes and expansions to the XHRExamples in UIExplorer. `onprogress` handler, `onreadystatechange` handler, and `responseType = 'arraybuffer'` can be switched on independently. I tested all combinations on both iOS and Android.

philikon · 2016-06-30T22:06:58Z

Thanks for the reviews and questions, everyone. Really appreciate it. If there are no more concerns or questions, would love it if somebody could pull the trigger and import it.

javache · 2016-07-01T10:46:19Z

@facebook-github-bot import

ghost · 2016-07-01T10:47:30Z

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

javache · 2016-07-01T12:13:47Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/ProgressResponseBody.java

+    private BufferedSource mBufferedSource;
+    private long mTotalBytesRead;
+
+    public ProgressResponseBody(ResponseBody responseBody, ProgressListener progressListener) {


I get an Infer warning here:

There may be a Field Not Initialized: Field ProgressResponseBody.mBufferedSource is not initialized in the constructor and is not declared @Nullable

Ah yeah, guess we should mark it @Nullable.

I can amend the commit here on github, but it seems @davidaurelio is already amending the diff in FB land. If David doesn't add it while he's in there, I can send a trivial PR after this lands.

davidaurelio · 2016-07-01T21:52:35Z

@philikon I’m on the internal failures. Making progress.

philikon · 2016-07-01T23:21:39Z

@philikon I’m on the internal failures. Making progress.

Eeek ok. Thanks! You know how to reach me if I can help with anything :)

philikon · 2016-07-05T20:22:51Z

@davidaurelio any updates? anything i can help with?

davidaurelio · 2016-07-05T21:40:34Z

I’ve added a comment here: #8324 (comment)

hard to spot, as it didn’t show up at the bottom. Any idea what could cause that?

davidaurelio · 2016-07-07T20:46:50Z

@philikon any idea what the underlying issue might be? @foghina can you take a look? --> #8324 (comment)

philikon · 2016-07-08T19:31:36Z

Thanks @davidaurelio! I replied to the comment.

ghost · 2016-07-12T20:49:44Z

Thank you for your pull request. As you may know, we require contributors to sign our Contributor License Agreement, and we don't seem to have you on file and listed as active anymore. In order for us to review and merge your code, please email cla@fb.com with your details so we can update your status.

…ase64 Summary: In preparation for Blob support (wherein binary XHR and WebSocket responses can be retained as native data blobs on the native side and JS receives a web-like opaque Blob object), this change makes RCTNetworking aware of the responseType that JS requests. A `xhr.responseType` of `''` or `'text'` translates to a native response type of `'text'`. A `xhr.responseType` of `arraybuffer` translates to a native response type of `base64`, as we currently lack an API to transmit TypedArrays directly to JS. This is analogous to how the WebSocket module already works, and it's a lot more versatile and much less brittle than converting a JS *string* back to a TypedArray, which is what's currently going on. Now that we don't always send text down to JS, JS consumers might still want to get progress updates about a binary download. This is what the `'progress'` event is designed for, so this change also implements that. This change also follows the XHR spec with regards to `xhr.response` and `xhr.responseText`: - if the response type is `'text'`, `xhr.responseText` can be peeked at by the JS consumer. It will be updated periodically as the download progresses, so long as there's either an `onreadystatechange` or `onprogress` handler on the XHR. - if the response type is not `'text'`, `xhr.responseText` can't be accessed and `xhr.response` remains `null` until the response is fully received. `'progress'` events containing response details (total bytes, downloaded so far) are dispatched if there's an `onprogress` handler. Once Blobs are landed, `xhr.responseType` of `'blob'` will correspond to the same native response type, which will cause RCTNetworking to only send a blob ID down to JS, which can then create a `Blob` object from that for consumers. Closes facebook/react-native#8324 Reviewed By: javache Differential Revision: D3508822 Pulled By: davidaurelio fbshipit-source-id: 441b2d4d40265b6036559c3ccb9fa962999fa5df

…ase64 Summary: In preparation for Blob support (wherein binary XHR and WebSocket responses can be retained as native data blobs on the native side and JS receives a web-like opaque Blob object), this change makes RCTNetworking aware of the responseType that JS requests. A `xhr.responseType` of `''` or `'text'` translates to a native response type of `'text'`. A `xhr.responseType` of `arraybuffer` translates to a native response type of `base64`, as we currently lack an API to transmit TypedArrays directly to JS. This is analogous to how the WebSocket module already works, and it's a lot more versatile and much less brittle than converting a JS *string* back to a TypedArray, which is what's currently going on. Now that we don't always send text down to JS, JS consumers might still want to get progress updates about a binary download. This is what the `'progress'` event is designed for, so this change also implements that. This change also follows the XHR spec with regards to `xhr.response` and `xhr.responseText`: - if the response type is `'text'`, `xhr.responseText` can be peeked at by the JS consumer. It will be updated periodically as the download progresses, so long as there's either an `onreadystatechange` or `onprogress` handler on the XHR. - if the response type is not `'text'`, `xhr.responseText` can't be accessed and `xhr.response` remains `null` until the response is fully received. `'progress'` events containing response details (total bytes, downloaded so far) are dispatched if there's an `onprogress` handler. Once Blobs are landed, `xhr.responseType` of `'blob'` will correspond to the same native response type, which will cause RCTNetworking to only send a blob ID down to JS, which can then create a `Blob` object from that for consumers. Closes facebook#8324 Reviewed By: javache Differential Revision: D3508822 Pulled By: davidaurelio fbshipit-source-id: 441b2d4d40265b6036559c3ccb9fa962999fa5df

…ase64 Summary: In preparation for Blob support (wherein binary XHR and WebSocket responses can be retained as native data blobs on the native side and JS receives a web-like opaque Blob object), this change makes RCTNetworking aware of the responseType that JS requests. A `xhr.responseType` of `''` or `'text'` translates to a native response type of `'text'`. A `xhr.responseType` of `arraybuffer` translates to a native response type of `base64`, as we currently lack an API to transmit TypedArrays directly to JS. This is analogous to how the WebSocket module already works, and it's a lot more versatile and much less brittle than converting a JS *string* back to a TypedArray, which is what's currently going on. Now that we don't always send text down to JS, JS consumers might still want to get progress updates about a binary download. This is what the `'progress'` event is designed for, so this change also implements that. This change also follows the XHR spec with regards to `xhr.response` and `xhr.responseText`: - if the response type is `'text'`, `xhr.responseText` can be peeked at by the JS consumer. It will be updated periodically as the download progresses, so long as there's either an `onreadystatechange` or `onprogress` handler on the XHR. - if the response type is not `'text'`, `xhr.responseText` can't be accessed and `xhr.response` remains `null` until the response is fully received. `'progress'` events containing response details (total bytes, downloaded so far) are dispatched if there's an `onprogress` handler. Once Blobs are landed, `xhr.responseType` of `'blob'` will correspond to the same native response type, which will cause RCTNetworking to only send a blob ID down to JS, which can then create a `Blob` object from that for consumers. Closes facebook/react-native#8324 Reviewed By: javache Differential Revision: D3508822 Pulled By: davidaurelio fbshipit-source-id: 441b2d4d40265b6036559c3ccb9fa962999fa5df

ghost 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 Jun 22, 2016

philikon force-pushed the xhr_responseType branch from ecec829 to 54ab996 Compare June 23, 2016 01:07

javache reviewed Jun 23, 2016
View reviewed changes

philikon force-pushed the xhr_responseType branch from 54ab996 to e676c28 Compare June 25, 2016 05:30

mkonicek assigned javache and davidaurelio Jun 28, 2016

javache reviewed Jun 28, 2016
View reviewed changes

foghina self-assigned this Jun 29, 2016

philikon force-pushed the xhr_responseType branch from e676c28 to 8c9e4dd Compare June 30, 2016 01:29

foghina reviewed Jun 30, 2016
View reviewed changes

philikon force-pushed the xhr_responseType branch from 8c9e4dd to 8eba055 Compare June 30, 2016 22:06

javache reviewed Jul 1, 2016
View reviewed changes

ghost closed this in 08c375f Jul 13, 2016

philikon deleted the xhr_responseType branch July 14, 2016 02:28

philikon mentioned this pull request Nov 24, 2016

Blobs #11103

Closed

7 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add responseType as a concept to RCTNetworking, send binary data as base64 #8324

Add responseType as a concept to RCTNetworking, send binary data as base64 #8324

philikon commented Jun 22, 2016 •

edited

Loading

ghost commented Jun 22, 2016

davidaurelio commented Jun 23, 2016

javache Jun 23, 2016

philikon Jun 23, 2016 •

edited

Loading

davidaurelio commented Jun 23, 2016

philikon commented Jun 23, 2016

davidaurelio commented Jun 23, 2016

philikon commented Jun 25, 2016

philikon commented Jun 27, 2016

mkonicek commented Jun 28, 2016

javache Jun 28, 2016

javache commented Jun 28, 2016

davidaurelio commented Jun 29, 2016

philikon commented Jun 30, 2016

foghina Jun 30, 2016

philikon Jun 30, 2016

foghina commented Jun 30, 2016

philikon commented Jun 30, 2016

philikon commented Jun 30, 2016

javache commented Jul 1, 2016

ghost commented Jul 1, 2016

javache Jul 1, 2016

philikon Jul 1, 2016

davidaurelio commented Jul 1, 2016

philikon commented Jul 1, 2016

philikon commented Jul 5, 2016

davidaurelio commented Jul 5, 2016

davidaurelio commented Jul 7, 2016

philikon commented Jul 8, 2016

ghost commented Jul 12, 2016

Add responseType as a concept to RCTNetworking, send binary data as base64 #8324

Add responseType as a concept to RCTNetworking, send binary data as base64 #8324

Conversation

philikon commented Jun 22, 2016 • edited Loading

ghost commented Jun 22, 2016

davidaurelio commented Jun 23, 2016

javache Jun 23, 2016

Choose a reason for hiding this comment

philikon Jun 23, 2016 • edited Loading

Choose a reason for hiding this comment

davidaurelio commented Jun 23, 2016

philikon commented Jun 23, 2016

davidaurelio commented Jun 23, 2016

philikon commented Jun 25, 2016

philikon commented Jun 27, 2016

mkonicek commented Jun 28, 2016

javache Jun 28, 2016

Choose a reason for hiding this comment

javache commented Jun 28, 2016

davidaurelio commented Jun 29, 2016

philikon commented Jun 30, 2016

foghina Jun 30, 2016

Choose a reason for hiding this comment

philikon Jun 30, 2016

Choose a reason for hiding this comment

foghina commented Jun 30, 2016

philikon commented Jun 30, 2016

philikon commented Jun 30, 2016

javache commented Jul 1, 2016

ghost commented Jul 1, 2016

javache Jul 1, 2016

Choose a reason for hiding this comment

philikon Jul 1, 2016

Choose a reason for hiding this comment

davidaurelio commented Jul 1, 2016

philikon commented Jul 1, 2016

philikon commented Jul 5, 2016

davidaurelio commented Jul 5, 2016

davidaurelio commented Jul 7, 2016

philikon commented Jul 8, 2016

ghost commented Jul 12, 2016

philikon commented Jun 22, 2016 •

edited

Loading

philikon Jun 23, 2016 •

edited

Loading