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

refactor: default to file scheme #866

Merged
merged 2 commits into from
May 27, 2020
Merged

Conversation

erisu
Copy link
Member

@erisu erisu commented May 25, 2020

Motivation and Context

Use file as default scheme to avoid break in current apps/

Description

  1. defaults to file if preference scheme is not set.
  2. continue to uses file if preference scheme is set to file.
  3. use what ever value the user defines in as preference scheme as a custom scheme handler, excluding file which was defined in the above case 2.
  4. preference hostname is only used when scheme is not file.
  5. CDV_ASSETS_URL is set to <scheme>://<hostname>
    -> if file, hostname = “”.
    -> if !file, hostname = "localhost" or user define value for preference hostname.
  6. Extension of above case 3, if scheme is set and is not file, it will also validate against WKWebView handlesURLScheme, if it is not valid, it will default to app

Testing

  • cordova build
  • run in simulator

Checklist

  • I've run the tests to see all new and existing tests pass

@erisu erisu added this to the 6.0.0 milestone May 25, 2020
@erisu erisu requested a review from dpogue May 25, 2020 06:55
@codecov-commenter
Copy link

codecov-commenter commented May 25, 2020

Codecov Report

Merging #866 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #866   +/-   ##
=======================================
  Coverage   74.40%   74.40%           
=======================================
  Files          13       13           
  Lines        1676     1676           
=======================================
  Hits         1247     1247           
  Misses        429      429           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7460737...653cbf9. Read the comment docs.

Copy link
Member

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked the code with my limited knowledge. LGTM 👍

@jcesarmobile jcesarmobile self-requested a review May 26, 2020 16:10
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should cheek if the appScheme is not null, otherwise it adds (null):// to allowNavigations
https://github.com/apache/cordova-ios/blob/master/CordovaLib/Classes/Private/Plugins/CDVIntentAndNavigationFilter/CDVIntentAndNavigationFilter.m#L48

Similar thing here https://github.com/apache/cordova-ios/blob/master/CordovaLib/Classes/Private/Plugins/CDVWebViewEngine/CDVWebViewEngine.m#L163
it makes window.CDV_ASSETS_URL to be"(null)://", we should probably just not set it so it's undefined.

Finally the convertFilePath function should check if window.CDV_ASSETS_URL is undefined and in that case just return the same path that was provided without modifications.

@erisu erisu requested a review from jcesarmobile May 27, 2020 02:54
@erisu erisu dismissed jcesarmobile’s stale review May 27, 2020 02:55

The feedback suggestions were applied

@erisu erisu force-pushed the cdv-use-file-default branch 2 times, most recently from 4251ee8 to ac2db4c Compare May 27, 2020 05:14
@erisu erisu force-pushed the cdv-use-file-default branch from ac2db4c to 653cbf9 Compare May 27, 2020 07:14
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@erisu erisu merged commit 1eb1846 into apache:master May 27, 2020
@erisu erisu deleted the cdv-use-file-default branch May 27, 2020 10:10
@ghost
Copy link

ghost commented Jun 26, 2020

I have a hard time understanding how to use custom scheme with cordova-ios 6, specifically the behaviour introduced here :

In the description of the behaviour by erisu, it says :

it will also validate against WKWebView handlesURLScheme, if it is not valid, it will default to app

But in the code, the actual condition is :

if(scheme == nil || [WKWebView handlesURLScheme:scheme]){
     scheme = @"app";
}

which, to my understanding of Objective-C, means that if it IS valid, it will default to app, cf. WKWebView handlesURLScheme documentation here

Could someone clarify this to me ?

@breautek
Copy link
Contributor

During the course of development, defaulting to custom scheme was changed to default to the file url.

The reasoning is because changing protocols will cause users to be missing their web storage data. So we wanted to make schemes an opt-in feature instead.

I don't think this feature is documented yet, but an example config to use schemes can be found at #781 (review)

@jcesarmobile
Copy link
Member

handlesURLScheme returns true if the webview already handles the scheme, if the webview already handles that scheme, it means we can't use that scheme for serving the app content, so if you set the scheme to http, https, file or a few others, handlesURLScheme is true and we default to app, if it returns false, it means whatever you configured is safe to use.

@lovelyelfpop
Copy link

lovelyelfpop commented Jul 20, 2020

Cordova-ios@6.1.0 says 'refactor: default to file scheme', why the url of my app is still myscheme:// not 'file://'?
How to open file:// url while still remain <preference name="scheme" value="myscheme" />?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants