breaking: Use WKURLSchemeHandler for serving app content #781

jcesarmobile · 2020-02-11T00:29:17Z

breaking because serving from a custom scheme makes the apps lose existing data (localStorage, etc)

Used app as default scheme, but I'm open to using cordova, cdv or any other.
The scheme is configurable with "scheme" preference, the hostname is configurable with "hostname" preference.
App will be loaded from "app://localhost"
If the scheme is changed, it has to be allowed with an allow-navigation entry, app is allowed on the default template.

Note that at the moment, loading file urls (in example, camera image in a src tag) won't work because of cross domain requests not allowed. What we did on ionic webview plugin was to create an utility function that converts file:// urls to app://localhost/ urls, but not sure how to do it here. Put it in cordova.js?

closes: #415

erisu · 2020-02-11T02:00:39Z

I think cordova.js might be OK for this utility. Not sure of anywhere else it could go.

You mentioned that it was implemented in the ionic webview plugin. We have the cordova-ios/cordova-js-src/plugin/ios/wkwebkit.js file which was the content of the cordova-plugin-wkwebview-engine JS logic and is bundled in the final output of cordova.js.

Maybe it could be placed in that file?

erisu

Code LGTM. 👍

I tested:

cordova platform add
cordova build ios
Tested with default config.xml
Tested with modified config.xml

<platform name="ios">
  <allow-navigation href="cdv://*" />
  <preference name="scheme" value="cdv" />
  <preference name="hostname" value="foobar" />
</platform>

Tested with CORS request remote
Tested with Fetch request remote
Tested with Fetch request local file

erisu · 2020-02-13T03:36:03Z

You can rebase this PR with master and it should fix the failing tests now.

I just merged in #782 which fixed the failing tests with the latest node. This will now unblock the reviewing/merging hold up.

erisu · 2020-02-13T11:48:24Z

/me bot did not see request but pre-confirmed, rebase complete.

knight9999 · 2020-02-13T09:08:33Z

CordovaLib/Classes/Private/Plugins/CDVIntentAndNavigationFilter/CDVIntentAndNavigationFilter.m

@@ -46,7 +46,7 @@ - (void)parser:(NSXMLParser*)parser didStartElement:(NSString*)elementName names
 - (void)parserDidStartDocument:(NSXMLParser*)parser
 {
    // file: url <allow-navigations> are added by default
-    self.allowNavigations = [[NSMutableArray alloc] initWithArray:@[ @"file://" ]];
+    self.allowNavigations = [[NSMutableArray alloc] initWithArray:@[ @"file://", @"app://" ]];


Is this "app://" scheme fixed? (In this case, do we expect that the user add allow-navigation directive in config.xml in order to allow his own scheme?)

I think it is better to use [settings cordovaSettingForKey:@"scheme"]; here instead of fixed "app://", if possible.

You can change the default by using the preference scheme option. But if they change, the must add allow-navigation

Good point, I thought to only allow the default one, but we can easily allow the configured one without an additional allow-navigation, will change it

Since the user can enter an invalid scheme (i.e. http, https, etc) that will default to "app", instead of reading directly from settings, I've created an appScheme in the view controller and use that for allowing the navigation.

…dova-ios into scheme-handlers

dpogue · 2020-02-18T05:00:20Z

breaking because serving from a custom scheme makes the apps lose existing data (localStorage, etc)

Is there an option to continue using file:/// URLs for apps that can't risk the data loss? Speaking personally, losing existing data would be unacceptable for at least 2 of my apps. I've already had to deal with data migration once from UIWebView to WKWebView.

knight9999

It looks good!

jcesarmobile · 2020-02-18T08:25:19Z

breaking because serving from a custom scheme makes the apps lose existing data (localStorage, etc)

Is there an option to continue using file:/// URLs for apps that can't risk the data loss? Speaking personally, losing existing data would be unacceptable for at least 2 of my apps. I've already had to deal with data migration once from UIWebView to WKWebView.

It could be done, but I did it this way because in your proposal you said “Update to use Scheme Handlers with WKWebView.”, not “add an option to use Scheme Handlers” or something like that. Also we have dropped iOS 10 support because of that.
And data can still be migrated, it’s just not handled by cordova itself, it has to be the developer who does the migration.
So maybe we should discuss in your proposal or in the dev list if we should keep serving from file as an option.

jcesarmobile · 2020-02-18T08:50:49Z

BTW, you should be able to install cordova-plugin-wkwebview-engine and that should make your app use file instead of a custom scheme.

erisu · 2020-02-25T09:20:31Z

@dpogue from your question/concern and @jcesarmobile response, is it OK to continue with merging this PR in?

Or do you have other questions?

Trying to get a status on this PR.

jcesarmobile · 2020-02-27T09:42:44Z

Going to merge since there is no response

In case we decide to add the file:// back as an option, it can be done in a separate PR, or cordova-plugin-wkwebview-engine can still be used

brodycj · 2020-02-27T14:20:08Z

Thanks for your work on this!

imselvakumar · 2020-05-19T18:43:12Z

Hello,

Able to retrieve app contents from remote http web page. But not able to retrieve app contents from remote https web page. Can you please help me on this.

NiklasMerz · 2020-05-20T06:25:02Z

Please go to Stack Overflow or Slack for questions.

Issues and pullrequests are not for questions.

imselvakumar · 2020-05-20T06:41:17Z

@NiklasMerz okay sure. Thanks!

lovelyelfpop · 2020-07-17T15:23:50Z

How to display local images in application storage directory (e.g. file:///var/mobile/Applications//) after add <preference name="scheme" value="cdv" /> <preference name="hostname" value="localhost" /> ?

lovelyelfpop · 2020-07-17T17:30:22Z

Got a solution, #947

Pratz007-tech · 2020-08-25T06:49:49Z

Hi with [email protected] I have set preferences with scheme: value="app" and hostname: value="localhost" because of which my origin in api calls is going as app:localhost resulting in cors issue i tried scheme: value="http" and hostname value="myDomain" expecting origin in api call as http:myDomain . But this is not working. Any suggestions would be appreciated :) Thanks !!

Pratz007-tech · 2020-08-25T07:38:54Z

Code LGTM. 👍

I tested:

cordova platform add

cordova build ios

Tested with default config.xml

Tested with modified config.xml
<platform name="ios">
  <allow-navigation href="cdv://*" />
  <preference name="scheme" value="cdv" />
  <preference name="hostname" value="foobar" />
</platform>
Tested with CORS request remote

Tested with Fetch request remote

Tested with Fetch request local file

Can I configure scheme & hostname like scheme as http & hostname as myDomain ? I tried but it still showed app:myDomain in origin while doing api calls.

timbru31 · 2020-08-25T08:46:08Z

Please seek support via asking on StackOverflow or in our Slack community.

erisu · 2020-08-25T09:44:08Z

No, there are reserved schemes that can not be used for example http, https, and file. If you set a scheme that is invalid (reserved), it will default to app.

This information had also been written already in this PR. Please read the comments as well before asking.

breaking: Use WKURLSchemeHandler for serving app content

a35a37b

jcesarmobile requested review from erisu, dpogue, knight9999 and NiklasMerz February 11, 2020 00:29

Add _app_file_ handling

d1bf609

erisu approved these changes Feb 12, 2020

View reviewed changes

erisu added the enhancement label Feb 12, 2020

erisu added this to the 6.0.0 milestone Feb 12, 2020

jcesarmobile added 2 commits February 13, 2020 20:47

breaking: Use WKURLSchemeHandler for serving app content

3395547

Add _app_file_ handling

a334d24

erisu force-pushed the scheme-handlers branch from d1bf609 to a334d24 Compare February 13, 2020 11:47

knight9999 reviewed Feb 15, 2020

View reviewed changes

jcesarmobile added 3 commits February 16, 2020 16:40

Merge branch 'scheme-handlers' of https://github.com/jcesarmobile/cor…

72b3472

…dova-ios into scheme-handlers

Allow navigation to the scheme used to serve the app

d6129a6

use scheme from vc

6df6cbe

knight9999 approved these changes Feb 18, 2020

View reviewed changes

jcesarmobile merged commit f09b879 into apache:master Feb 27, 2020

jcesarmobile deleted the scheme-handlers branch February 27, 2020 09:43

breautek mentioned this pull request Jun 26, 2020

refactor: default to file scheme #866

Merged

1 task

apache locked as resolved and limited conversation to collaborators Aug 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking: Use WKURLSchemeHandler for serving app content #781

breaking: Use WKURLSchemeHandler for serving app content #781

jcesarmobile commented Feb 11, 2020 •

edited by erisu

Loading

erisu commented Feb 11, 2020 •

edited

Loading

erisu left a comment •

edited

Loading

erisu commented Feb 13, 2020

erisu commented Feb 13, 2020

knight9999 Feb 13, 2020

erisu Feb 15, 2020

jcesarmobile Feb 15, 2020

jcesarmobile Feb 16, 2020

dpogue commented Feb 18, 2020

knight9999 left a comment

jcesarmobile commented Feb 18, 2020

jcesarmobile commented Feb 18, 2020

erisu commented Feb 25, 2020

jcesarmobile commented Feb 27, 2020

brodycj commented Feb 27, 2020

imselvakumar commented May 19, 2020

NiklasMerz commented May 20, 2020

imselvakumar commented May 20, 2020

lovelyelfpop commented Jul 17, 2020

lovelyelfpop commented Jul 17, 2020

Pratz007-tech commented Aug 25, 2020

Pratz007-tech commented Aug 25, 2020

timbru31 commented Aug 25, 2020

erisu commented Aug 25, 2020 •

edited

Loading

breaking: Use WKURLSchemeHandler for serving app content #781

breaking: Use WKURLSchemeHandler for serving app content #781

Conversation

jcesarmobile commented Feb 11, 2020 • edited by erisu Loading

erisu commented Feb 11, 2020 • edited Loading

erisu left a comment • edited Loading

Choose a reason for hiding this comment

erisu commented Feb 13, 2020

erisu commented Feb 13, 2020

knight9999 Feb 13, 2020

Choose a reason for hiding this comment

erisu Feb 15, 2020

Choose a reason for hiding this comment

jcesarmobile Feb 15, 2020

Choose a reason for hiding this comment

jcesarmobile Feb 16, 2020

Choose a reason for hiding this comment

dpogue commented Feb 18, 2020

knight9999 left a comment

Choose a reason for hiding this comment

jcesarmobile commented Feb 18, 2020

jcesarmobile commented Feb 18, 2020

erisu commented Feb 25, 2020

jcesarmobile commented Feb 27, 2020

brodycj commented Feb 27, 2020

imselvakumar commented May 19, 2020

NiklasMerz commented May 20, 2020

imselvakumar commented May 20, 2020

lovelyelfpop commented Jul 17, 2020

lovelyelfpop commented Jul 17, 2020

Pratz007-tech commented Aug 25, 2020

Pratz007-tech commented Aug 25, 2020

timbru31 commented Aug 25, 2020

erisu commented Aug 25, 2020 • edited Loading

jcesarmobile commented Feb 11, 2020 •

edited by erisu

Loading

erisu commented Feb 11, 2020 •

edited

Loading

erisu left a comment •

edited

Loading

erisu commented Aug 25, 2020 •

edited

Loading