Skip to content

Conversation

@animaonline
Copy link

Fixes #18223

Test Plan

fetch('https://apipre.monkimun.com/whoami', {
  body: null,
  method: 'HEAD',
  headers: {
    Accept: 'application/json',
    'Content-Type': 'application/json',
  },
})
  .then(response => response.json())
  .then(json => console.log(json));

Additional information can be found in the issue #18223

Related PRs

Release Notes

CATEGORY

[ANDROID] [BUGFIX] [XMLHttpRequest] - Fixed an issue where fetch HEAD requests would fail with Invalid response for blob
[IOS] [BUGFIX] [XMLHttpRequest] - Fixed an issue where fetch HEAD requests would fail with Invalid response for blob

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 22, 2018
@react-native-bot react-native-bot added 📋Release Notes 🌐Networking Related to a networking API. Platform: iOS iOS applications. labels Mar 22, 2018
@janicduplessis
Copy link
Contributor

Do you think it would be better to not parse the body as a blob when the method is HEAD instead? I wonder what the spec says about that.

Otherwise we might end up returning null when it should actually throw.

@animaonline
Copy link
Author

As far as I know there’s no body in a HEAD request.

@janicduplessis
Copy link
Contributor

@animaonline Yea, HEAD responses don't have a body so my question is why are we trying to parse it as a blob. I think fixing that would be better.

@janicduplessis
Copy link
Contributor

Alright so it looks like whatwg-fetch uses blob whenever it is available (https://github.com/github/fetch/blob/27473e937c6bdbb61212cc078ad60d22ae77ffb1/fetch.js#L454).

Not sure if this is ideal, but XMLHttpRequest should be able to handle null body + responseType blob. I read through the spec and it doesn't really explicitly mention how to handle it but I tested it in chrome and it does return a blob instance of size 0 and type text/plain. It seems like this is also what the Android implementation does.

I suggest we fix the iOS implementation so it also returns an empty blob. Furthermore we should investigate if having the responseType always set to blob when using fetch could cause issues.

@animaonline
Copy link
Author

It should definitely be fixed in the future, but for now I believe this will do, this issue is critical, it also seems to happen when executing a GET fetch request that is expected to return JSON, but returns html (for example a 404 page)., or when executing a fetch request while offline.

My PR takes care of that problem, but I agree that it should be handled in a better way.

@xiamx
Copy link

xiamx commented Mar 24, 2018

This patch is identical to the one I proposed on #18223 (comment) 10 days ago for #18223 ... Yet, no attribution.

The solution proposed here is only a workaround that masks the source of the issue. The creation of _cachedResponse should be fixed for the case of empty body instead.

@animaonline
Copy link
Author

Well, the logic in this "patch" is similar to how XMLHttpRequest handles other response types, as I've written before this bug also applies to GET requests, and in our case it's really critical, you don't really expect me to attribute you for 2 lines of code that borrows from the existing logic, do you?

@janicduplessis
Copy link
Contributor

@animaonline Thanks for giving a shot at fixing this, I submitted a new patch that fixes the underlying issue in the iOS implementation, so this workaround won't be needed. See #18547 for a more detailed explanation of the issue.

@peacechen
Copy link

This error also manifests with RN Android. This PR should be applied as a safety net in case the underlying platform generates an empty blob. While an empty blob may not be in spec, this is causing fatal errors in multiple platforms (iOS and Android, has anyone test Windows?). Expecting all platforms to be maintained equally isn't a good assumption and makes RN more brittle.

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. 🌐Networking Related to a networking API. Platform: iOS iOS applications.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants