-
Notifications
You must be signed in to change notification settings - Fork 24.9k
Fixed 0.54 - iOS] fetch HEAD requests fail with Invalid response for blob
#18500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
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. |
|
As far as I know there’s no body in a HEAD request. |
|
@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. |
|
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. |
|
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. |
|
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 |
|
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? |
|
@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. |
|
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. |
Fixes #18223
Test Plan
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