[RFC] Add support for missing XHR response types by davidaurelio · Pull Request #6870 · react/react-native

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

 }

+ 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

+ 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

- 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 notifications@github.com 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

+}
+
+/*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:Fixesreact#6679 This adds support for the missing response types to XMLHttpRequest. Don?t ship this yet. This is completely untested. yolo and stuff. Closesreact#6870 Reviewed By: bestander Differential Revision: D3153628 Pulled By: davidaurelio fb-gh-sync-id: 76feae3377bc24b931548a9ac1af07943b1048ac fbshipit-source-id: 76feae3377bc24b931548a9ac1af07943b1048ac

…eact#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 fcc89e9Apr 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 PRr…
46982db
…eact#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 PRr…
e0808e8
…eact#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 PRr…
2969bc8
…eact#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 PRr…
97174fc
…eact#6870

This pull request was closed.

Uh oh!

Conversation

davidaurelio commented Apr 7, 2016

Uh oh!

facebook-github-bot commented Apr 7, 2016

Uh oh!

davidaurelio commented Apr 7, 2016

Uh oh!

davidaurelio commented Apr 7, 2016

Uh oh!

facebook-github-bot commented Apr 7, 2016

Uh oh!

eslint-botApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

davidaurelioApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

bestander commented Apr 7, 2016

Uh oh!

davidaurelio commented Apr 7, 2016

Uh oh!

ideApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

davidaurelioApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

ide commented Apr 7, 2016

Uh oh!

facebook-github-bot commented Apr 7, 2016

Uh oh!

eslint-botApr 7, 2016

Choose a reason for hiding this comment

Uh oh!

davidaurelio commented Apr 8, 2016

Uh oh!

facebook-github-bot commented Apr 8, 2016

Uh oh!

davidaurelio commented Apr 8, 2016

Uh oh!

davidaurelio commented Apr 8, 2016

Uh oh!

facebook-github-bot commented Apr 8, 2016

Uh oh!

davidaurelio commented Apr 8, 2016

Uh oh!

facebook-github-bot commented Apr 8, 2016

Uh oh!

facebook-github-bot commented Apr 8, 2016

Uh oh!

davidaurelio commented Apr 8, 2016

Uh oh!

bestander commented Apr 8, 2016

Uh oh!

ide commented Apr 8, 2016

Uh oh!

ide commented Apr 8, 2016

Uh oh!

javacheApr 8, 2016

Choose a reason for hiding this comment

Uh oh!

davidaurelioApr 10, 2016

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 11, 2016

Uh oh!

davidaurelio commented Apr 11, 2016

Uh oh!

facebook-github-bot commented Apr 11, 2016

Uh oh!

davidaurelio commented Apr 11, 2016

Uh oh!

facebook-github-bot commented Apr 11, 2016

Uh oh!

facebook-github-bot commented Apr 11, 2016

Uh oh!

ide commented Apr 11, 2016

Uh oh!

mpretty-cyro commented May 3, 2016