Skip to content
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

In Android not able to read Arabic translation JSON file using JSONStream #51

Closed
midhunm-c opened this issue Jul 12, 2021 · 19 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@midhunm-c
Copy link

midhunm-c commented Jul 12, 2021

Arabic translation JSON file is downloaded to document directory and it is reading and parsing using RNFetchBlob.JSONStream.

Issue:
In android when we tried to read Arabic translation file using JSONStream the callbacks done or fail in not getting called. And not throwing any exceptions.
In iOS JSONStream is calling done block once the same Arabic json file read complete.

Please note: I have checked this with joltup/rn-fetch-blob
but I also tried to install the latest version of this library, and the result is same.

@RonRadtke
Copy link
Owner

Could you please provide a sample json that is problematic?
Don't need you full translation file, just a few lines that will cause the problem.

@midhunm-c
Copy link
Author

@RonRadtke
this is a portion of the json file which makes problem.

{
    "app/screens/onboarding/onboarding-button-texts/next": "Next",
    "app/screens/contact/send-bulk-email": "إرسال بريد إلكتروني دعائي"
  }

This is added to a json file in android document directory and tried to read using JSONStream. done or fail block not getting called.

If I removed this Arabic text from the json the JSONStream is calling the done block.

@midhunm-c
Copy link
Author

@RonRadtke
While reading the json file with Arabic text from application document directory as JSONStream this following exception was throwing

image

I have fixed the issue in this PR, #53
Please review this PR.

@RonRadtke RonRadtke added the bug Something isn't working label Aug 1, 2021
@RonRadtke
Copy link
Owner

Thank you for your PR.
My problem is, it is not actually fixing the problem. It's a simple bad aid working for now. But in general there seems to be something off with either the data itself or what happens to the data before it being encoded.
So either there are characters in which are not covered by utf8 or somewhere goes something wrong.
So I'll have a closer look at the problem as soon as I have time for.

Also I don't like to have different handling for the different encodings. If we go for it, it would be better to ignore the encoding errors for all different types of encoding to have a similar behavior all over the code.

@devinm-hrbl
Copy link

Hello! I completely understand your comments, in fact I would say the same thing. That being said, is there any chance we can have a beta version with this quick fix until you can fix the actual problem? I'd really rather not fork this repo (yet again) and re-host just to have a temporary fix, and I don't have the time to invest in exploring other avenues for a fix.

@RonRadtke
Copy link
Owner

I pushed your fix on a feature branch. Due to the merge conflict I just copied it quickly.
It's the branch features/arabic_json_fix

So you should be able to install it by running npm install RonRadtke/react-native-blob-util#feature/arabic_json_fix

@devinm-hrbl
Copy link

Wonderful! Thank you so much, this is going to save us a lot of trouble!

@RonRadtke
Copy link
Owner

I'm relatively sure I know the cause of the problem now.
Basically there are only two options:

  • Your file contains non-utf8 chars
  • Something in the Lib is broken

I assume your file is fine, otherwise you would most likely have problems in iOS too.
Currently we are splitting the input in chunks of 4096 Bytes, but this doesn't respect the fact that UTF8 uses 1,2 or 3 bytes to encode a single character. So most likely we are cutting a valid character in half and thus you're seeing the error.
I hope I'll have the time to reimplement that part in the next week.

I would like to ask you to test the changes with your file to verify it's working fine - I hope you could help there @devinm-hrbl / @midhunm-c ?
I would come back to you then, as soon as it's implemented.

@midhunm-c
Copy link
Author

Thank you very much @RonRadtke !!
Yeah sure, we can definitely test once you implemented the fix.

@RonRadtke
Copy link
Owner

RonRadtke commented Aug 21, 2021

@midhunm-c would it be a problem for you if I bump the minSDK to 19? I think I have a working solution for > APi 19.
That would on the other hand mean, you have to stay on a dev branch till I make the next major release, which I plan for September. (Hopefully I get the android scoped storage working till then)

Don't need to bump the API for this :) So I think you will get a branch to test tomorrow.

@RonRadtke
Copy link
Owner

@midhunm-c branch is ready for you.
You can install it via npm install RonRadtke/react-native-blob-util#feature/utf8_fix

@midhunm-c
Copy link
Author

midhunm-c commented Aug 26, 2021

Thank you @RonRadtke !!
I have checked from the branch feature/utf8_fix and the language is loading without any exception. 👍

@RonRadtke
Copy link
Owner

Glad to hear :)

@RonRadtke
Copy link
Owner

@devinm-hrbl I published a new version including the fix.
Please shortly give me hint as soon as you migrated so I can delete the old branch you were using.

@RonRadtke RonRadtke reopened this Aug 29, 2021
@RonRadtke RonRadtke self-assigned this Aug 29, 2021
@midhunm-c
Copy link
Author

Hi @RonRadtke thank you for the new release :)
I have checked from the new version v0.13.13.
But I am getting an infinite warning in console with this new version in both Android and iOS

Possible Unhandled Promise Rejection (id: 1292): TypeError: undefined is not a function (near '...ReactNativeBlobUtil.config...')
But I remember this warning were not in the last branch you have shared feature/utf8_fix and ![image](https://user-images.githubusercontent.com/87301308/131387786-cb46be7c-fb4c-41fb-9bcc-8c9b3bd4f951.png).

Screenshot:
image

PS:
I got this warning before when I was creating the PR from the forked repo. And I changed the import in polyfill/XMLHttpRequest.js.

Screenshot for ref:
image

@RonRadtke
Copy link
Owner

Yeah you're right - I messed that one up when fixing the import cycles.
Version 0.13.14 should fix it. It's already live.

@midhunm-c
Copy link
Author

Thank you @RonRadtke !!
Integrated the version 0.13.14 and now it's working fine :)

@RonRadtke
Copy link
Owner

Good - then I close the issue and delete the branch within the next days :)

@devinm-hrbl
Copy link

Thank you so much @RonRadtke , and great collaboration @midhunm-c!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants