-
Notifications
You must be signed in to change notification settings - Fork 759
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
fix(ios): Entry.toURL() to produce DOM-usable uri when using scheme-hosted webview #642
Conversation
I'm putting this in draft. Good news is, this PR actually addresses that failing test that we've had for some time:
Bad news is, it introduces a lot more failing tests but I think most of them is because the tests are expecting |
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.
This seems good to me.
Taking a step back (and keeping in mind that it's been several years since I used the File plugin): it's not clear to me what toURL()
and nativeURL
are meant to do exactly, and there doesn't seem to be any documentation beyond the deprecated W3C spec that says "toURL()
returns a URL" and has no mention of nativeURL
.
My initial assumption is that toURL()
is intending to return a DOM-usable URL, and nativeURL
is the raw filesystem path, but that sounds like it's not necessarily the case?
I believe once upon a time,
|
3dde5a9
to
cd7b96a
Compare
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.
Code wise looks good to me.
Platforms affected
iOS
Motivation and Context
Entry's
toURL()
method is intended to produce a DOM-usable URIs.This works as intended on Android.
On iOS however, it only worked if the app was configured to be in a
file://
hosted webview. If the webview was configured to use schemes, it would still produce afile://
uri which is not usable in the DOM.I believe this can be considered a patch, for
file://
hosted users, this should behave the same as before. For scheme-hosted users, the.toURL()
will return a different URI, but the original format didn't work as intended. If the user were using.toURL()
for another purpose, then this could be considered a breaking change?Description
Mimick how Android solves this issue, by creating an ios specific
Entry.js
file which contains atoURL
method override with iOS specific logic.Testing
Manually tested with and without the
scheme
preference to observetoURL()
producingfile://
urls while infile://
hosted mode, orapp://
uris if the app is usingscheme
preference.Also tested completely custom schemes using:
and observing URIs such as
myapp://appname/_app_file_/var/mobile/Containers/Data/Application/84D10AA8-302C-4295-8690-54F4C4EA9BB2/tmp/cdv_photo_1730252303.jpg
.Confirmed that in both cases, the produced URIs are usable in the DOM to properly load image assets, for example.
Unit Test Refactoring
Now that
.toURL()
returns aapp://
uri instead of afile://
uri, it broke several tests that used.toURL()
to resolve a file entry.app://
uris return an encoding error, presumably because there is no filesystem implementation to handle the scheme.Most cases I've replaced the usage of
.toURL()
with.nativeURL
to make the test pass. However there is a handful of tests that was specifically calling.toNativeURL()
to test the resolve method and.toNativeURL()
simply calls.toURL()
with a deprecation notice. I think these tests can be complete as I believe they are an out-dated practice..toNativeURL()
is also deprecated, but I've kept them for now, just disabled them until we decide what to do.Checklist
(platform)
if this change only applies to one platform (e.g.(android)
)