Make XMLHttpRequest and XMLHttpRequest.upload proper EventTargets by philikon · Pull Request #7017 · facebook/react-native

philikon · 2016-04-17T03:34:10Z

So far, XHR only supports a few onfoo event handlers, not the entier EventTarget interface (addEventListener, removeEventListener). It also doesn't support the upload object on Android -- for no good reason. Even if we don't send any events there yet, there's no reason we have to break consuming code that wants to register an event handler there. This PR rectifies all that.

Fortunately, adding proper EventTarget support is very easy thanks to event-target-shim. We already use it in our WebSocket implementation. It transparently handles the addEventListener('foo', ...) as well as onfoo APIs, so when you dispatch an event on the event target, the right handlers will be invoked. The event object is wrapped so that event.target is set properly. Basically, it's a super easy way to make us conform to the spec.

Also added a bit of polish here and there, using ES2015 class property goodness to consolidate a lot of Flow property definitions with the corresponding property initializers.

Test Plan: Ran through the XHR Example in UIExplorer on both platforms. Made sure upload progress events still work on iOS and timeouts work.

facebook-github-bot · 2016-04-17T03:35:18Z

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

mkonicek · 2016-04-19T11:17:02Z

Is this one good to go? Should I merge it?

sreesharp · 2016-04-19T12:39:49Z

Code looks good to me. Hope Flow is not complaining about the XHRBase static attributes.

philikon · 2016-04-19T17:41:43Z

Flow didn't complain. Should be good to go.

grgmo · 2016-04-20T08:58:04Z

looks good to me, good to merge

mkonicek · 2016-04-20T14:57:25Z

@facebook-github-bot shipit

facebook-github-bot · 2016-04-20T15:03:57Z

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

mkonicek · 2016-04-20T16:34:03Z

@philikon Looks like this breaks fetching QEs in one of our internal apps. Maybe @andreicoman11 knows what's wrong?

philikon · 2016-04-20T20:39:19Z

Looks like this breaks fetching QEs in one of our internal apps.

Weird. In an end-to-end test or in a unit test? If it's a unit test, I could imagine that the unit test uses some internal knowledge of XMLHttpRequestBase to stub out some stuff.

Anyway, I'd love to help in whatever way I can to fix that. You guys know where to find me :)

mkonicek · 2016-04-21T10:47:23Z

Philipp I've just sent you a message :)

philikon · 2016-04-21T20:19:19Z

I've just looked over the diff again. There's very little behavioral change to XHR, so I'm surprised there's a new failure.

The biggest change is obviously concerning the EventTarget API. So one possible failure scenario could be something this:

existing code does something like xhr.addEventListener && xhr.addEventListener('timeout', timeoutHandler) and sets a timeout (or is on a platform with a default timeout)
in the test, the request actually times out
the consumer would never have found out before this patch, but now it does, causing the test to fail

The other change is that event handlers now get an event object rather than null. Not that they should care, but who knows...

Wish I could be more helpful. If it's too difficult to track down the problem on your end, @mkonicek, we can also try to take this diff in steps and see which change creates the failure. Would be tedious, but perhaps in the end quicker? Your call.

TanTan-TT · 2016-04-23T06:23:23Z

@philikon I have a question after reading RN Android code. RN Android networking code did not handle upload progress event, so your change can not make uploading progress works. Am I right?

philikon · 2016-04-25T17:10:43Z

@philikon I have a question after reading RN Android code. RN Android networking code did not handle upload progress event, so your change can not make uploading progress works. Am I right?

Correct, this doesn't change the supported functionality on either platform.

mkonicek · 2016-04-25T18:11:04Z

@philikon I'll try installing fb4a in my emulator and running the running the test locally to repro the timeout.

ghost · 2016-04-26T20:12:17Z

@philikon updated the pull request.

eslint-bot · 2016-04-26T20:18:45Z

Libraries/Network/XMLHttpRequestBase.js

no-undef: 'EventListener' is not defined.

ghost · 2016-04-26T20:26:40Z

@philikon updated the pull request.

ghost · 2016-04-27T16:22:14Z

@philikon updated the pull request.

philikon · 2016-04-27T17:11:04Z

(circleci failure unrelated)

mkonicek · 2016-04-27T19:07:17Z

@facebook-github-bot shipit

ghost · 2016-04-27T19:12:45Z

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

ghost · 2016-04-28T19:15:21Z

@philikon updated the pull request.

philikon · 2016-04-28T19:16:44Z

Rebased on top of D3229435 / d363b1f. Can somebody plz re-import kthxbai :)

mkonicek · 2016-04-29T14:01:46Z

#finallylanded 👍

mkonicek · 2016-04-29T14:02:00Z

Thanks for the help Kevin!

Summary: So far, XHR only supports a few `onfoo` event handlers, not the entier `EventTarget` interface (`addEventListener`, `removeEventListener`). It also doesn't support the `upload` object on Android -- for no good reason. Even if we don't send any events there yet, there's no reason we have to break consuming code that wants to register an event handler there. This PR rectifies all that. Fortunately, adding proper `EventTarget` support is very easy thanks to `event-target-shim`. We already use it in our WebSocket implementation. It transparently handles the `addEventListener('foo', ...)` as well as `onfoo` APIs, so when you dispatch an event on the event target, the right handlers will be invoked. The event object is wrapped so that `event.target` is set properly. Basically, it's a super easy way to make us conform to the spec. Also added a bit of polish here and there, using ES2015 class property goodness to consolidate a lot of Flow property definitions with the corresponding property initializers. **T Closes facebook#7017 Reviewed By: fkgozali Differential Revision: D3202021 Pulled By: martinbigio fb-gh-sync-id: 2b007682074356c75c774fab337672918b6c4355 fbshipit-source-id: 2b007682074356c75c774fab337672918b6c4355

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

philikon force-pushed the xhr_upload branch from 08996e7 to a11d6fc Compare April 17, 2016 20:34

philikon mentioned this pull request Apr 17, 2016

Fix issue where xhr.upload is undefined on Android #6986

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

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Apr 21, 2016

philikon force-pushed the xhr_upload branch from a11d6fc to 2ab2309 Compare April 26, 2016 20:11

eslint-bot reviewed Apr 26, 2016
View reviewed changes

Libraries/Network/XMLHttpRequestBase.js Outdated

Copy link

eslint-bot Apr 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no-undef: 'EventListener' is not defined.

philikon force-pushed the xhr_upload branch from 2ab2309 to f3972ba Compare April 26, 2016 20:26

philikon force-pushed the xhr_upload branch from f3972ba to c57be97 Compare April 27, 2016 16:21

mkonicek removed the Import Failed label Apr 27, 2016

ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Apr 27, 2016

ghost added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Apr 28, 2016

Make XMLHttpRequest and XMLHttpRequest.upload proper EventTargets

e158f25

philikon force-pushed the xhr_upload branch from c57be97 to e158f25 Compare April 28, 2016 19:14

ghost closed this in b5f14ea Apr 28, 2016

mkonicek removed the Import Failed label May 6, 2016

philikon deleted the xhr_upload branch June 8, 2016 00:29

philikon mentioned this pull request Nov 24, 2016

Blobs #11103

Closed

7 tasks

This pull request was closed.

Conversation

philikon commented Apr 17, 2016

Uh oh!

facebook-github-bot commented Apr 17, 2016

Uh oh!

mkonicek commented Apr 19, 2016

Uh oh!

sreesharp commented Apr 19, 2016

Uh oh!

philikon commented Apr 19, 2016

Uh oh!

grgmo commented Apr 20, 2016

Uh oh!

mkonicek commented Apr 20, 2016

Uh oh!

facebook-github-bot commented Apr 20, 2016

Uh oh!

mkonicek commented Apr 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philikon commented Apr 20, 2016

Uh oh!

mkonicek commented Apr 21, 2016

Uh oh!

philikon commented Apr 21, 2016

Uh oh!

TanTan-TT commented Apr 23, 2016

Uh oh!

philikon commented Apr 25, 2016

Uh oh!

mkonicek commented Apr 25, 2016

Uh oh!

ghost commented Apr 26, 2016

Uh oh!

eslint-bot Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 26, 2016

Uh oh!

ghost commented Apr 27, 2016

Uh oh!

philikon commented Apr 27, 2016

Uh oh!

mkonicek commented Apr 27, 2016

Uh oh!

ghost commented Apr 27, 2016

Uh oh!

ghost commented Apr 28, 2016

Uh oh!

philikon commented Apr 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkonicek commented Apr 29, 2016

Uh oh!

mkonicek commented Apr 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mkonicek commented Apr 20, 2016 •

edited

Loading

philikon commented Apr 28, 2016 •

edited

Loading