Skip to content

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

Closed
ryanlntn wants to merge 1 commit intofacebook:masterfrom
ryanlntn:fix/xhr-upload-undefined
Closed

Fix issue where xhr.upload is undefined on Android#6986
ryanlntn wants to merge 1 commit intofacebook:masterfrom
ryanlntn:fix/xhr-upload-undefined

Conversation

@ryanlntn
Copy link
Contributor

When attempting to set xhr.upload.onprogress on Android I get the following error:
undefined is not an object (evaluating 'xhr.upload')

Looking at the iOS implementation of XMLHTTPRequest I found the following code:

  constructor() {
    super();
    // iOS supports upload
    this.upload = {};
  }

Applying the same constructor to the Android implementation resolves the issue.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot 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 14, 2016
@dsibiski
Copy link
Contributor

@ryanlntn Thanks for the PR! I was looking at XMLHttpRequestBase.js to see if we could consolidate the duplicate code into this base component and I saw this:

constructor() {
    ...
    this.upload = undefined; /* Upload not supported yet */
    ...
}

XMLHttpRequestBase.js#L92

I wonder what the comment means? Is that still true? It appears to be working with the fix here...

@nicklockwood @mkonicek @lexs @philikon any thoughts?

@philikon
Copy link
Contributor

Yeah, I don't see any reason why we can't at least expose the upload object on Android. It doesn't say anywhere that we have to support all events on it.

I actually have a work-in-progress to make XHR a proper EventTarget: philikon@5bb05bb

@philikon
Copy link
Contributor

Ok, I cleaned up my patched and submitted it as a PR: #7017.

@ghost
Copy link

ghost commented May 8, 2016

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

@philikon
Copy link
Contributor

philikon commented May 9, 2016

We can close this PR, it's been superseded by #7017.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Infinite Red Partner: Infinite Red Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants