-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
[RCTNetworking] Received data was invalid. #1780
Comments
@brentvatne Do you have any idea here? I was now debugging for some hours but had no luck until now. |
@nicklockwood I just figured it was your commit in RCTNetworking.m that introduced this check (f88bc3e) - do you have any idea as to why this might happen? |
We currently only support XHR requests that return text responses. Support for images is planned, but not currently implemented. It probably worked before because we didn't report the error and just returned null as the response. |
@nicklockwood Thanks for the answer. Would it make sense to add it to the breaking changes section then? Moreover, wouldnt it be better to log the error but not create a redbox? Sometimes, you might just want to open an URL without parsing the result (as it is the case for google analytics sometimes). |
Yes, on both counts. |
@brentvatne I've seen that you have been active concerning "breaking changes" recently IIRC. Are you able to add stuff there? Concerning the other point of changing: if (!responseText && data.length) {
RCTLogError(@"Received data was invalid.");
return;
} in RCTNetworking.m to something that does not result in a Redbox: could you gimme some guidance as to how this should be done in RN? Is it enough to change "RCTLogError" to "RCTLogWarn"? I would like to add a PR then (have never done that before, so a little more guidance is highly appreciated :)) |
Hi Philipp, I'm just investigating what would be involved. If it was a normal method call you'd just return an error callback, but in this case it's slightly more complicated as you'd need to cancel the request to prevent multiple errors being sent. |
@nicklockwood OK, I've just created a pull request to work with, feel free to give feedback in there if thats better. |
Thanks for the heads up @PhilippKrone, I updated the breaking changes section of the release notes: https://github.com/facebook/react-native/releases/tag/v0.7.0-rc |
Thanks for looking into this guys, as @PhilippKrone said, this is affecting my GA module. |
Can we not just check the |
I just updated to 0.7.0-rc.2 and started getting some response errors on the app that I'm creating. I submitted a pull request at #1917 that fixes a bug in the way response headers are looked up (values were supposed to be lowercased but ended up not being lowercased which caused looking up things like I'm not sure if this is going to fix your problem but since you said it was working prior to 0.7.0-rc.2 and my change fixes a bug introduced in this commit (e00b9ac) that was introduced in 0.7.0-rc.2, I figured you could try it out! |
Summary: As discussed with @nicklockwood in the issue facebook#1780, the error should be changed to a warning to not break fetch() to send a POST to a remote API without wanting to parse the reply. E.g. google sends back an empty 1x1px gif when POSTing something to google analytics. Closes facebook#1860 Github Author: "philipp.krone" <[email protected]>
FYI, I've merged the RCTWarn solution internally for now. It's not the right fix, but it will do as a workaround for the time being. @negativetwelve the lowercase content-type bug is a separate issue, which has also now been fixed. Should be in the next update. |
I'm also experiencing "Received data was invalid." on a response with Content-Type "text/html" and response.text() yields empty string "" |
Might be malformed utf8 - can you paste in the url you're trying to download? |
I've been seeing this same issue with |
@nicklockwood I dug more into this, and, the reason in my case is that |
Ascii is a subset of UTF8, so that shouldn't be a problem. I suspect the issue here is that the text includes some characters that mean it doesn't conform to any valid encoding, and Apple's string parsing is super-strict about bad encoding. I'll see if I can find a way to make it more robust/tolerant. |
@gauravmc can you include some examples of pages that it won't download properly so I can test? |
@nicklockwood Since the pages I was requesting are behind my login session, here's a super tiny server you can test this against: https://github.com/gauravmc/encoding_test_server |
Summary: @public RCTNetworking currently relies on network responses to include an accurate text encoding, otherwise it is unable to convert the response data to text unless it's encoded as UTF8. See: #1780 (comment) for details. This diff makes use of a new feature in iOS8 to detect the encoding of the text authomatically Reviewed By: @sahrens Differential Revision: D2443446
I've added a fix, but it relies on an iOS 8-only API I'm afraid: 0044b3c Feel free to keep this issue open if iOS 7 support is critical for you. |
Summary: @public RCTNetworking currently relies on network responses to include an accurate text encoding, otherwise it is unable to convert the response data to text unless it's encoded as UTF8. See: facebook#1780 (comment) for details. This diff makes use of a new feature in iOS8 to detect the encoding of the text authomatically Reviewed By: @sahrens Differential Revision: D2443446
@nicklockwood awesome, thanks! I'd imagine iOS 7 support being important for people; what do you think? |
It's not that it isn't important, but it's going to be very complex to fix if Apple don't provide an API for it, and we'll probably be dropping iOS 7 support within a year or so now that iOS 9 is out (assuming it's adopted quickly). |
Summary: @public RCTNetworking currently relies on network responses to include an accurate text encoding, otherwise it is unable to convert the response data to text unless it's encoded as UTF8. See: facebook#1780 (comment) for details. This diff makes use of a new feature in iOS8 to detect the encoding of the text authomatically Reviewed By: @sahrens Differential Revision: D2443446
Closing this since it's already fixed for ios 8+. Feel free to submit a PR with ios 7 support. |
@chirag04 can you reopen this? It appears people are still having an issue with this. rnc-archive/react-native-google-analytics#19 (comment) |
Still having this issue on iOS 9.1 from a Google Analytics request as @lwansbrough mentioned above. |
+1 |
My team is having an issue with running PouchDB in React Native and retrieving images from CouchDB. Error message: react-native 0.26 |
@PaulMest if you want to retrieve images via a network request, you'll have to do this via the |
@nicklockwood The reason why this is affecting me is actually out of my control. I'm sending Google Analytics data to Google's collect URL, and they send back a 1x1 image. Is there anyway we can "dangerously allow" binary content? |
@nicklockwood This is helpful guidance. I'll chat with my team about it. Thanks. For more context: we're building an offline-first app that can do bi-directional syncing when the device comes online. We're trying to leverage PouchDB/CouchDB for this. The performance is acceptable now with hundreds of images and we have resorted to base64 encoding images and syncing the strings. Not a very elegant solution by any means, but it's functional. If we had more time, we could probably figure out a way to build our own offline-to-online bi-directional sync provider for images while keeping them native. But I agree with @lwansbrough that there should be a way to "dangerously allow" binary content. |
@lwansbrough I'll look into it. |
@nicklockwood I suppose I could use the aforementioned image pre fetching API? All my data is being passed as query parameters anyway. |
@lwansbrough note that unless you actually need to prefetch the image, you can simply store the URL and pass it directly to |
@nicklockwood Unfortunately the current API doesn't make use of the rendering context (nor do I think it should, particularly). In fact it doesn't require anything from React Native. I still think optionally delegating control of the response binary to the user might be the best solution in my case. |
@nicklockwood How long do images stay in the prefetch cache for? Would it be a bad choice to use that for downloading 1px images from GA? |
@nicklockwood on iOS it's an LRU cache with a fixed size limit (200MB on disk, 5MB in memory). Images will stay in the cache indefinitely until they're pushed out by a new image. |
Thanks for the info! |
2018 and the yellow box warning continues to pop up when using Google Analytics. It's "just" a warning, but when multiple warnings consistently stack up, developers start to ignore all warnings and it lessens the value of warnings in general. Image.prefetch isn't an option for us because our app can't waste cache space on these empty pixels. Would like to see an option in the config object (second param to
|
Hi,
since v0.7.0-rc.2, I get the following error:
When trying to fetch() something as follows:
This code was working in prior versions of RN. The return from google seems to be an empty gif (1x1px).
P.S. this is from react-native-google-analytics.
The text was updated successfully, but these errors were encountered: