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

feat(android): add support for opening file:// URIs for reading #16

Merged
merged 1 commit into from
Apr 20, 2021
Merged

feat(android): add support for opening file:// URIs for reading #16

merged 1 commit into from
Apr 20, 2021

Conversation

MegaMaddin
Copy link

Most popular RN 3rd party libraries handling files (e.g. image/photo handling)
are returning file:// URIs on Android. To save handling this in JS several
times, we can simply parse the input string as an URI and use the path part to
open files.

Signed-off-by: Martin Mazein internet+github@mazein.net

Testing done

Added an additional test in the Example App. Also tested the patch within my apps, which handle file URIs from e.g. react-native-image-cropper-picker.

@alpha0010
Copy link
Owner

If adding support this way, read calls on android will support file://, but from what I can tell, other calls will not (e.g. rename file). Does using plain paths still work?

We should also support uri scheme on iOS.

@MegaMaddin
Copy link
Author

Does using plain paths still work?

Yes, I didn't change those calls in the example app, and I didn't saw an error there. We could add this to rename as well?

We should also support uri scheme on iOS.

Which one do you have in mind? I'm currently working on support for ph:// URIs, which is also "just" a react-native thing (3rd party libs and RN are using them to resolve to the internal photo library).

@alpha0010
Copy link
Owner

alpha0010 commented Apr 19, 2021

Yes, I didn't change those calls in the example app, and I didn't saw an error there.

Hmm. Looks like it works by parse yields an empty scheme when none is provided. Valid URIs require scheme, so I think this works by relying on implementation behavior instead of specs https://developer.android.com/reference/android/net/Uri#parse(java.lang.String) . Perhaps safer to confirm path looks URI-ish before using it as such?

We could add this to rename as well?

Perhaps add a helper function that returns a File object given a path. Helper can parse URI (when needed) and construct the object. Replacing all other File(...) constructor calls with the helper should insure all APIs support both standard and URI paths.

Which one do you have in mind? I'm currently working on support for ph:// URIs, which is also "just" a react-native thing (3rd party libs and RN are using them to resolve to the internal photo library).

At least file://, so as to match Android. I am fine if that is a separate PR.

Thanks for your work on this.

@MegaMaddin
Copy link
Author

Yep, I can add the support for making the distinction between URIs and file paths.

At least file://, so as to match Android. I am fine if that is a separate PR.

Sure, I can look into that.

Regardless of that - do you have any plans to integrate some testing into the lib?
It would be nice to have a some base coverage for the available calls.

@alpha0010
Copy link
Owner

Tests would be great, but I have not written tests targeting native code in react native before and am not certain how to approach. Writing jest tests for the js side does not currently feel useful, since basically the only js logic is adding default parameters to native calls.

Any suggestions?

@MegaMaddin
Copy link
Author

MegaMaddin commented Apr 19, 2021

Any suggestions?

Not really, the first thing which comes to my mind would be Firebase test-lab for E2E testing. But that requires a lot of preliminary setup. We could start with simply unit testing both native code bases in the first places.

Other than that, updated the series.

Most popular RN 3rd party libraries handling files (e.g. image/photo handling)
are returning file:// URIs on Android. To save handling this in JS several
times, we can simply try to detect if we are handling an URI and parse them
accordingly. Regardless of the input we always return an File object to handle
files.

Signed-off-by: Martin Mazein <internet+github@mazein.net>
@alpha0010 alpha0010 mentioned this pull request Apr 20, 2021
@alpha0010 alpha0010 merged commit f02992b into alpha0010:master Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants