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: drop uiwebview & add wkwebview #773

Merged
merged 7 commits into from
Feb 10, 2020

Conversation

erisu
Copy link
Member

@erisu erisu commented Jan 29, 2020

Motivation, Context, Description

  • Removes UIWebView
  • Integrates cordova-plugin-wkwebview-engine
  • Renames WKWebView engine plugin classes to not conflict with Ionic.
    • Prefixed w/ CDVWebView:
    • Notice: Plugins should not use the follow prefix CDV. It is typically used for Cordova's internal class naming convention. If a plugin happens to uses the same class name, it will create conflicts. As well, if any changes were made on Cordova's end and happens to break the plugin, the plugin would need to make the corrections because the plugin decided to use and conflict with Cordova's internal naming convention.
  • Removes WK_WEB_VIEW_ONLY preference options.
  • fixes: Remove all UIWebView code? #661
  • closes: Fix #661: Removed all UIWebView code. #663

Testing

  • npm t
  • cordova platform add
  • cordova requirements ios
  • cordova prepare ios
  • cordova build ios --release --device
    • Uploaded Manually Built IPA
    • Launch Manual Uploaded IPA
  • cordova run ios --release --emulator
  • IAB test
    • cordova plugin add cordova-plugin-inappbrowser@3.2.0
    • cordova build ios --release --device
      • Uploaded Manually Built IPA
      • Launch Manual Uploaded IPA

Checklist

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

@erisu erisu changed the title refactor: drop uiwebview & default to wkwebview refactor: drop uiwebview & add wkwebview Jan 29, 2020
@codecov-io
Copy link

codecov-io commented Jan 29, 2020

Codecov Report

Merging #773 into master will decrease coverage by 0.15%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
- Coverage   74.42%   74.26%   -0.16%     
==========================================
  Files          11       11              
  Lines        1830     1815      -15     
==========================================
- Hits         1362     1348      -14     
+ Misses        468      467       -1
Impacted Files Coverage Δ
bin/templates/scripts/cordova/lib/prepare.js 83.74% <100%> (-0.32%) ⬇️

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 266d339...8ace315. Read the comment docs.

@erisu erisu added this to the 6.0.0 milestone Jan 29, 2020
@NiklasMerz NiklasMerz self-requested a review January 29, 2020 09:47
cordova-js-src/exec.js Outdated Show resolved Hide resolved
Copy link
Member

@dpogue dpogue left a comment

Choose a reason for hiding this comment

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

My understanding is that the following code from CDVAppDelegate has no effect with WKWebView and can also be removed: https://github.com/apache/cordova-ios/blob/master/CordovaLib/Classes/Public/CDVAppDelegate.m#L28-L38

@@ -20,11 +20,7 @@
#import <Cordova/CDVPlugin.h>
#import <Cordova/CDVWhitelist.h>

#if WK_WEB_VIEW_ONLY
#define CDVWebViewNavigationType int
Copy link
Member

Choose a reason for hiding this comment

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

Does this get used anywhere by WKWebView or should it be removed entirely?

Copy link
Member Author

Choose a reason for hiding this comment

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

@erisu erisu force-pushed the refactor/swap-webviews branch from 5374728 to fd54ace Compare February 1, 2020 06:58
@erisu erisu requested a review from dpogue February 6, 2020 23:54
@erisu erisu force-pushed the refactor/swap-webviews branch from 905b1f1 to cab13af Compare February 7, 2020 01:01
@erisu erisu force-pushed the refactor/swap-webviews branch from cab13af to 94f46d3 Compare February 7, 2020 01:32
@erisu erisu requested a review from knight9999 February 7, 2020 04:59
Copy link
Contributor

@knight9999 knight9999 left a comment

Choose a reason for hiding this comment

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

I confirmed that test app works properly in my Mac environment.
Supposing other plugins those using WK_WEB_VIEW_ONLY flag will be updated soon according to this PR,
#773 (review)
I think there is no problem.

Keeping & forcing the WK_WEB_VIEW_ONLY flag to true will allows the IAB plugin @3.2.x to continue to function.
This flag is kept only for the transitioning period.
!! This flag will be removed on Cordova iOS's following next major (7.x)
@erisu erisu force-pushed the refactor/swap-webviews branch from 8840270 to 8ace315 Compare February 7, 2020 09:07
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.

I think it's a good idea to set WK_WEB_VIEW_ONLY for now. We can remove that later (once InAppBrowser is ready).

I tested this PR in an app which is using WKWebView and IAB and it works fine.

@erisu erisu merged commit 240a177 into apache:master Feb 10, 2020
@erisu erisu deleted the refactor/swap-webviews branch February 10, 2020 04:32
@jpike88
Copy link

jpike88 commented Feb 16, 2020

@erisu now that this has been merged when can we look forward to next release?

@brodycj
Copy link
Contributor

brodycj commented Feb 17, 2020

If you need this update now, I think your best bet at this point would be to install from GitHub, for example:

  • cordova platform add github:apache/cordova-ios
  • cordova platform add github:apache/cordova-ios#bce99f94

There should also be a nightly tag on npm. In general, we do not actively support unstable versions from master or from nightly.

My understanding is that there are some breaking changes still in progress before we are ready to publish the new release.

In case you have any questions about the upcoming release I would highly recommend you followup with us on Slack or on the mailing list, follow links in the footer of cordova.io or cordova.apache.org.

NiklasMerz pushed a commit to GEDYSIntraWare/cordova-ios that referenced this pull request Feb 24, 2020
* refactor: drop uiwebview & default to wkwebview
* refactor: remove CDVLocalStorage
* refactor: set CDVWebViewNavigationTypeOther as -1
* refactor: udpated & fixed cordova.js
* refactor: force WK_WEB_VIEW_ONLY flag to true

Note:
* The WK_WEB_VIEW_ONLY flag is hardcoded to true. This is to ensure that the IAB@3.2.x plugin is forced to use WKWebView as this release does not support UIWebView.
* The WK_WEB_VIEW_ONLY flag is kept only as part of a transitioning period. The flag will be removed in a later release.
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.

Remove all UIWebView code?
7 participants