-
Notifications
You must be signed in to change notification settings - Fork 992
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
Support file:// and library content:// files #49
base: master
Are you sure you want to change the base?
Support file:// and library content:// files #49
Conversation
@jrichardlai Thanks! I'm not an Android-person but I guess @cjdell has an opinion on this :) |
@jrichardlai Are you using this in production? I'm inclined to merge this as @cjdell isn't responding. But I cannot test it on my own right now, so I want to make sure that you are positive that this works as expected. Is there anything you think we should add to the readme/docs for this? |
@johanneslumpe I'm using it in production didn't see any problems so far, but this feature is not used a lot. Also I added this change because it makes sense for us ( we use it when we want to retry an upload that fails, which can be from either a image from the library or a new photo ) but I don't know if it fits in this library if react-native-file-system is really about only the file system. |
And yeah a second opinion would be better :) |
Accepting a subset or URLs using existing entry points is confusing and could introduce correctness issues because this API is specified as accepting file paths. (e.g. why don't the other functions in the API accept URLs, only this one? Why are only some kinds of URLs accepted? What if I really want to write a file that looks like a URL in some custom Android distribution, but this code prevents me from doing so?) |
@mcorb word. Maybe I should make another method for that |
I'm attempting to open pdfs downloaded by using Linking.openUrl and it fails to open if I use a file://path_to_file Would this PR make that function? I'll check it out, but if you can confirm that would be great. Edit: I suppose my thought that it was some kind of permissions issue was not correct. For anyone else looking for reason why files cannot be opened, this PR didn't solve not being able to open local files downloaded with RNFS via Linking. Everyone else, sorry for cluttering! |
@grokbot Thanks for posting your finding about |
This works for me. Thanks @jrichardlai |
This certainly fixes and issue with Android, but I agree that it should be implement on all methods so the support is consistent. |
Hi. Just pinging to check if any decision has been made on this PR. Thanks! |
@@ -75,15 +123,13 @@ public void writeFile(String filepath, String base64Content, ReadableMap options | |||
@ReactMethod | |||
public void readFile(String filepath, Callback callback) { | |||
try { | |||
File file = new File(filepath); | |||
|
|||
if (!file.exists()) throw new Exception("File does not exist"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add please again the File-Exists-check and throw an Exception, if the file does not exists? To not throw an exception would be compatibility break for all those that rely on this check in the thrown exception (instead of calling .exists() by themself in JS-layer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also recommend not adding file:// without checking if the filepath already starts with file:// to avoid double file://file://
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It actually doesn't start with file://
since the uri scheme is null
Thanks for reaching out. Just one thing. Please consider my comment |
@grokbot Could you elaborate on this? I'm using: [Edit]: It looks like the issue I was running into was the "...exposed beyond app through Intent.getData()" issue. I haven't found a way to get around this using Linking, but am awaiting this PR to get fixed before using on react-native-fetch-blob to get merged in in order to use it: joltup/rn-fetch-blob#58 |
@TomMahle in my case when I was working with this, I had previously downloaded the file and stored it in the default directory through the app, where I did run into some read problems when using Linking.openURL(). Changing the directory to ExternalDirectoryPath had cleared the initial problem of throwing errors of not having permission to open that file. If you're having permission issues, I think the app has to have same user as the target directory or more open permissions (which I think ExternalDirectoryPath is the latter) |
Not sure if that's the best way, but it allows reading file from library or filesystem.