[RFC] Add support for missing XHR response types #6870

davidaurelio · 2016-04-07T22:56:08Z

This adds support for the missing response types to XMLHttpRequest.
Don’t ship this yet. This is completely untested. yolo and stuff.

facebook-github-bot · 2016-04-07T22:57:14Z

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

davidaurelio · 2016-04-07T22:57:22Z

@vjeux @nicklockwood @javache @ide @mkonicek @bestander @satya164

davidaurelio · 2016-04-07T23:01:09Z

@facebook-github-bot import

facebook-github-bot · 2016-04-07T23:01:43Z

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

eslint-bot · 2016-04-07T23:01:46Z

Libraries/Network/XMLHttpRequestBase.js

    this._sent = false;
    this._lowerCaseResponseHeaders = {};

    this._clearSubscriptions();
  }

+  get responseType() {
+    return this._responseType;


get/set properties not yet supported

bestander · 2016-04-07T23:04:21Z

Your lint neglect is shameful

davidaurelio · 2016-04-07T23:08:11Z

wanna take over?

ide · 2016-04-07T23:08:14Z

Libraries/Network/XMLHttpRequestBase.js

+        break;
+
+      case 'blob':
+        this._cachedResponse = new global.Blob([this.responseText]);


The Blob constructor accepts some options including the MIME type of the response -- should we parse the HTTP response headers and pass the Content-Type to the Blob too?

I guess that makes sense

ide · 2016-04-07T23:18:03Z

Looks mostly good to me. I didn't look at the UTF-8 decoder too closely but assume it's right.

facebook-github-bot · 2016-04-07T23:24:19Z

@davidaurelio updated the pull request.

eslint-bot · 2016-04-07T23:29:34Z

Libraries/Network/XMLHttpRequestBase.js

@@ -44,8 +55,6 @@ class XMLHttpRequestBase {
  readyState: number;
  responseHeaders: ?Object;
  responseText: ?string;
-  response: ?string;
-  responseType: '' | 'text';
  status: number;
  timeout: number;


get/set properties not yet supported

davidaurelio · 2016-04-08T10:17:31Z

Thanks for the review!

I didn't look at the UTF-8 decoder too closely but assume it's right.

it actually doesn’t handle surrogate pairs.

facebook-github-bot · 2016-04-08T19:20:24Z

@davidaurelio updated the pull request.

davidaurelio · 2016-04-08T19:23:21Z

@facebook-github-bot shipit

davidaurelio · 2016-04-08T19:23:55Z

@bestander, this is ready for merge. I assume you have to accept it in phabricator

facebook-github-bot · 2016-04-08T19:26:17Z

@davidaurelio updated the pull request.

davidaurelio · 2016-04-08T19:26:39Z

@facebook-github-bot import

facebook-github-bot · 2016-04-08T19:26:53Z

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

facebook-github-bot · 2016-04-08T20:39:30Z

@davidaurelio updated the pull request.

davidaurelio · 2016-04-08T21:39:56Z

@ide, @bestander: as @javache assumed, ArrayBuffer / Uint8Array are unavailable on iOS7, even though they are activated in Safari.

What shall we do? Not support xhr.responseType = 'arraybuffer' at all? Setting it will throw an error on iOS7 right now. Android has support for it, I just checked the open source version

bestander · 2016-04-08T21:54:49Z

Do we even support ios7?
Otis like 2012

On Friday, 8 April 2016, David Aurelio [email protected] wrote:

@ide https://github.com/ide, @bestander https://github.com/bestander:
as @javache https://github.com/javache assumed, ArrayBuffer / Uint8Array
are unavailable on iOS7, even though they are activated in Safari.

What shall we do? Not support xhr.responseType = 'arraybuffer' at all?
Setting it will throw an error on iOS7 right now. Android has support for
it, I just checked the open source version

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6870 (comment)

ide · 2016-04-08T21:57:17Z

Let's support arraybuffer responses on platforms that support it so that we don't hold back React Native for an OS with small market share. Apple says < 5% of users are on iOS 7.

This is the behavior I believe we want:

Responses of type arraybuffer work on Android and iOS 8+
React Native should not crash on iOS 7 if you never get an arraybuffer response
If you do get an arraybuffer response on iOS 7, we should fail quickly and with a clear error

This should work for apps that want to target iOS 7 and you can easily write code like: if (isIOS7) { dontFetchAnArrayBuffer(); }. And for apps that are iOS 8+ you get the functionality you expect.

ide · 2016-04-08T22:05:34Z

@davidaurelio also could you add a comment to the code mentioning iOS 7 so that when we drop support for iOS 7 some day, it's easy to grep for deletable code?

javache · 2016-04-08T22:56:25Z

Libraries/Utilities/utf8.js

+}
+
+/*eslint-disable no-bitwise */
+exports.encode = (string: string): ArrayBuffer => {


I might be missing something here, but doing utf8 ourself doesn't seem like something we should do.

Like the #1 rule of crypto "don't roll your own", I think this applies to string encoding too.

I think the complexity of crypto and character encodings is very different. I have been looking for a compact utf-8 encoding lib, but didn’t find anything that was compact and solid at the same time. My implementation adds ~1KB when minified.

Do you have any specific concerns? We wouldn’t need to do this if we could pass the binary response data from native to JS wrapped into a host object we’d implement. That host object could pass of character encoding to a system service. I have been looking into JSC’s API for that, but as long as we pass messages as JSON, we can’t do it.

facebook-github-bot · 2016-04-11T11:03:18Z

@davidaurelio updated the pull request.

davidaurelio · 2016-04-11T11:03:45Z

@facebook-github-bot import

facebook-github-bot · 2016-04-11T11:04:11Z

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

davidaurelio · 2016-04-11T11:10:03Z

@facebook-github-bot import

facebook-github-bot · 2016-04-11T11:10:16Z

@davidaurelio updated the pull request.

facebook-github-bot · 2016-04-11T11:10:37Z

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

ide · 2016-04-11T19:05:56Z

Cherry-picking this into 0.24-stable.

Summary:Fixes #6679 This adds support for the missing response types to XMLHttpRequest. Don?t ship this yet. This is completely untested. yolo and stuff. Closes #6870 Reviewed By: bestander Differential Revision: D3153628 Pulled By: davidaurelio fb-gh-sync-id: 76feae3377bc24b931548a9ac1af07943b1048ac fbshipit-source-id: 76feae3377bc24b931548a9ac1af07943b1048ac

mpretty-cyro · 2016-05-03T00:15:23Z

@davidaurelio Just FYI the utf8 module this provides is supplanting a utf8 node module (https://github.com/mathiasbynens/utf8.js); I assume this is because of the '@providesModule' - should this really be doing that if all it does is encode?

davidaurelio · 2016-05-03T12:31:41Z

@mpretty-homepass sorry about that :-( we’ll get rid of the special packager rule for react-native.

Feel free to send a PR that renames the module to something else.

Summary:Fixes facebook#6679 This adds support for the missing response types to XMLHttpRequest. Don?t ship this yet. This is completely untested. yolo and stuff. Closes facebook#6870 Reviewed By: bestander Differential Revision: D3153628 Pulled By: davidaurelio fb-gh-sync-id: 76feae3377bc24b931548a9ac1af07943b1048ac fbshipit-source-id: 76feae3377bc24b931548a9ac1af07943b1048ac

…acebook#6870

Add support for missing XHR response types

f5c6126

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

eslint-bot reviewed Apr 7, 2016
View reviewed changes

ide reviewed Apr 7, 2016
View reviewed changes

Clean up and add type annotations

d667a6c

eslint-bot reviewed Apr 7, 2016
View reviewed changes

Fix utf8 encoding, pass content-type to Blob

673d302

Use invariant when checking responseType

411f590

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

Add helpful comments

046800c

javache reviewed Apr 8, 2016
View reviewed changes

Flow + comments

f8b10ab

Syntax

7bf8627

ghost closed this in fcc89e9 Apr 11, 2016

skellock mentioned this pull request Apr 23, 2016

Support For React Native Android infinitered/reactotron#33

Closed

wellmonge added a commit to wellmonge/react-native that referenced this pull request Feb 9, 2019

removing incoherent test at XMLHttpReuqesT due the change made by PR f…

46982db

…acebook#6870

wellmonge added a commit to wellmonge/react-native that referenced this pull request Feb 9, 2019

removing incoherent test at XMLHttpReuqesT due the change made by PR f…

e0808e8

…acebook#6870

cpojer pushed a commit to wellmonge/react-native that referenced this pull request Feb 11, 2019

removing incoherent test at XMLHttpReuqesT due the change made by PR f…

2969bc8

…acebook#6870

cpojer pushed a commit to wellmonge/react-native that referenced this pull request Feb 11, 2019

removing incoherent test at XMLHttpReuqesT due the change made by PR f…

97174fc

…acebook#6870

This pull request was closed.

[RFC] Add support for missing XHR response types #6870

[RFC] Add support for missing XHR response types #6870

Conversation

davidaurelio commented Apr 7, 2016

facebook-github-bot commented Apr 7, 2016

davidaurelio commented Apr 7, 2016

davidaurelio commented Apr 7, 2016

facebook-github-bot commented Apr 7, 2016

eslint-bot Apr 7, 2016

Choose a reason for hiding this comment

davidaurelio Apr 7, 2016

Choose a reason for hiding this comment

bestander commented Apr 7, 2016

davidaurelio commented Apr 7, 2016

ide Apr 7, 2016

Choose a reason for hiding this comment

davidaurelio Apr 7, 2016

Choose a reason for hiding this comment

ide commented Apr 7, 2016

facebook-github-bot commented Apr 7, 2016

eslint-bot Apr 7, 2016

Choose a reason for hiding this comment

davidaurelio commented Apr 8, 2016

facebook-github-bot commented Apr 8, 2016

davidaurelio commented Apr 8, 2016

davidaurelio commented Apr 8, 2016

facebook-github-bot commented Apr 8, 2016

davidaurelio commented Apr 8, 2016

facebook-github-bot commented Apr 8, 2016

facebook-github-bot commented Apr 8, 2016

davidaurelio commented Apr 8, 2016

bestander commented Apr 8, 2016

ide commented Apr 8, 2016

ide commented Apr 8, 2016

javache Apr 8, 2016

Choose a reason for hiding this comment

davidaurelio Apr 10, 2016

Choose a reason for hiding this comment

facebook-github-bot commented Apr 11, 2016

davidaurelio commented Apr 11, 2016

facebook-github-bot commented Apr 11, 2016

davidaurelio commented Apr 11, 2016

facebook-github-bot commented Apr 11, 2016

facebook-github-bot commented Apr 11, 2016

ide commented Apr 11, 2016

mpretty-cyro commented May 3, 2016

davidaurelio commented May 3, 2016