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

Fix #661: Removed all UIWebView code. #663

Closed
wants to merge 14 commits into from
Closed

Conversation

bpresles
Copy link

@bpresles bpresles commented Sep 2, 2019

Build Status

Removed all UIWebView related code.
Replaced default engine by CDVWKWebViewEngine.

Platforms affected

Motivation and Context

I've tested the pull request from #662, but the AppStore Connect still reported "ITMS-90809: Deprecated API Usage - Apple will stop accepting submissions of apps that use UIWebView APIs." with it (I tripled check that the WKWebViewOnly preference was correctly taken in account).

Also I don't think that doing a half way solution at compile time, instead of removing the UIWebView code altogether, is a good solution.
If the UIWebView is deprecated, than remove the code altogether as it'll be useless in the near future. People wanting to still use UIWebView can either use a UIWebView engine as a plugin or use older version of cordova-ios.

#661

Description

Removed all UIWebView code and replaced default engine by CDVWKWebViewEngine from cordova-plugin-wkwebview-engine (making it useless now).

Testing

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@bpresles bpresles changed the title Fix #661: Removed all UIWebView code. WIP: Fix #661: Removed all UIWebView code. Sep 2, 2019
@bpresles bpresles closed this Sep 2, 2019
@bpresles bpresles reopened this Sep 3, 2019
@bpresles bpresles changed the title WIP: Fix #661: Removed all UIWebView code. Fix #661: Removed all UIWebView code. Sep 3, 2019
@fjaeger
Copy link

fjaeger commented Sep 4, 2019

I don't see why #662 should not work if the preference is set correctly. All UIWebview-related code is encapsulated in preprocessor conditions.
Maybe you have some other plugin in your Cordova project that still contains traces to UIWebView?

@bpresles
Copy link
Author

bpresles commented Sep 4, 2019

I don't see why #662 should not work if the preference is set correctly. All UIWebview-related code is encapsulated in preprocessor conditions.
Maybe you have some other plugin in your Cordova project that still contains traces to UIWebView?

I added this line on the iOS platform section in my config.xml:

<preference name=“WKWebViewOnly” value=“true” />

Then I removed the iOS platform altogether and added it back, and made sure on XCode that the WK_WEB_VIEW_ONLY setting was added in the user defined build settings and set to 1 which was the case.

Then I cleaned the build folder and built and ran the app which was working just fine using WKWebview from cordova-plugin-wkwebview-engine.

So I make an Archive and submitted the app to AppStore Connect. And I still got the warning. I tried to completely remove the node_modules folder as well as the plugins folder to make sure that they get fetched again, and remove iOS platform than readded, but it was no better (the app worked fine but AppStore Connect still reported the UIWebView deprecation warning).

Then I worked on removing all reference of UIWebView in cordova-ios and removed the embedded CDVUIWebViewEngine, at first still using the cordova-plugin-wkwebview-engine plug-in without embedding it, and it was only then I could send an archive that didn’t trigger the deprecation warning (so without changing anything in my installed plugins). And then I worked on embedding the cordova-ios and removed the embedded CDVUIWebViewEngine, at first still using the cordova-plugin-wkwebview-engine directly in cordova-ios as default engine.

Also I think that using an optional compile time preference isn’t a good idea. Because Apple will not just deprecate the UIWebView APIs (it’s already been deprecated for a while) but reject all apps using them in the future, so it’ll not be very useful to keep this old UIWebView based engine as default.
If anyone want to still use UIWebView they can simply use previous versions of cordova-ios that are using it as default engine, or make and use an engine plugin that uses UIWebView (which can be done very easily and quickly by embedding the code of CDVWebViewEngine and associated JavaScript files from previous cordova-ios in a plugin).

In a nutshell, as UIWebView has been deprecated for a while now and that Apple will reject apps using these UIWebView APIs in the future, it’s not logical to keep a UIWebView based engine as default, it’s WKWebView that should be the default and UIWebView an option.

… plugin forked from cordova-plugin-wkwebview-engine
@EdwinRozario
Copy link

Will this PR be merged anytime soon or are we all going to install @bpresles 's fork?

@timbru31
Copy link
Member

timbru31 commented Sep 9, 2019

We haven't decided yet how to handle Apple's deprecation warning, please see (and participate :)) in the discussion here: apache/cordova-discuss#110

@rajashekaranugu
Copy link

@davidecastello
Copy link

Any news on this? This would be really appreciated, thanks.

@adminy
Copy link

adminy commented Nov 1, 2019

This seems to be mostly done, why hasn't this been merged yet? Apple has been giving the notice for a while now.

@@ -6,9 +6,9 @@
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the extra space added here and on line 11

@brodycj
Copy link
Contributor

brodycj commented Nov 1, 2019

Thanks guys for the contribution and the followup. This is under discussion in apache/cordova-discuss#110, and there are multiple blocking factors right now:

  • we still need some more time to reach agreement, and 100% consensus would be ideal
  • we need to have a committer review the changes
  • committers including myself are pretty overloaded

While I would agree that this is pretty urgent, we would appreciate a little bit of slack and some benefit of the doubt. I would rephrase the followup question something like this: "What is the status? This looks pretty urgent."

Guys please DO feel free to follow up with us from time to time. I did just push the discussion in this comment: apache/cordova-discuss#110 (comment)

I would also recommend you guys consider following up via mailing list or slack, our contact is here: https://cordova.apache.org/contact/

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

It looks to me like a few modules were deleted and a few others were added. I have a feeling it should be possible to use git mv to rename modules as needed before making the changes. This would save us a lot of time when reviewing.

One more suggestion for the next time is to raise a PR from a new branch, not from your master branch. https://blog.jasonmeridth.com/posts/do-not-issue-pull-requests-from-your-master-branch/

Copy link
Contributor

@brodycj brodycj left a comment

Choose a reason for hiding this comment

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

PR #662 seems to be a much simpler solution.

braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Dec 3, 2019
braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Dec 4, 2019
braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Dec 5, 2019
braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Dec 5, 2019
braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Dec 5, 2019
braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Dec 9, 2019
@erisu
Copy link
Member

erisu commented Jan 9, 2020

@bpresles Can you rebase this PR with master and resolve the conflicts?

Also, is it possible to renamed the classes with the prefix of CDVWebView instead of CDVIOSWK?

Since UIWebView is deprecated and wont be accepted in the AppStore for new apps starting in April, it means there will eventually be only one WebView (WKWebView).

@dpogue @jcesarmobile Do you have any opinions on using CDVWebView as the prefix instead?

@jcesarmobile
Copy link
Member

I think at this point is simpler to create a new PR, since the conflicts are due to the conditional compiler flag and it's going to take more time to sync all those files than creating a new PR.
Or we can revert the conditional flag PR since it's not going to be part of next release

braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Jan 22, 2020
braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Jan 24, 2020
braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Jan 24, 2020
braginxv pushed a commit to braginxv/cordova-ios that referenced this pull request Jan 24, 2020
@erisu erisu closed this in #773 Feb 10, 2020
@TakaKeiji
Copy link

Any news about this?
I'm trying to upload my app to TestFlight and since mid april 2020 the UIWebView deprecation restriction is active and rejecting any app with references to UIWebView

@breautek
Copy link
Contributor

breautek commented May 2, 2020

@TakaKeiji

I cannot comment about when 6.0 will be released but you can handle the uiwebview issue on 5.1.1

see https://cordova.apache.org/howto/2020/03/18/wkwebviewonly.html to learn how.

@TakaKeiji
Copy link

@TakaKeiji

I cannot comment about when 6.0 will be released but you can handle the uiwebview issue on 5.1.1

see https://cordova.apache.org/howto/2020/03/18/wkwebviewonly.html to learn how.

Thanks for the quick response, i'll try your recommendation later!

@TakaKeiji
Copy link

@TakaKeiji

I cannot comment about when 6.0 will be released but you can handle the uiwebview issue on 5.1.1

see https://cordova.apache.org/howto/2020/03/18/wkwebviewonly.html to learn how.

Working correctly now, the build was accepted correctly, thanks for the info, anyway i'm still having issues with the oficial archived PayPal mobile sdk cordova plugin, so i'm looking a solution for this now...

@ls-1N
Copy link

ls-1N commented May 21, 2020

@TakaKeiji
I cannot comment about when 6.0 will be released but you can handle the uiwebview issue on 5.1.1
see https://cordova.apache.org/howto/2020/03/18/wkwebviewonly.html to learn how.

Working correctly now, the build was accepted correctly, thanks for the info, anyway i'm still having issues with the oficial archived PayPal mobile sdk cordova plugin, so i'm looking a solution for this now...

I tried to find the source, but couldn't - I just read on of these issues that Paypal SDK is deprecated and users should move to BrainTree or one other solution.

@breautek
Copy link
Contributor

The paypal plugin provided by PayPal itself appears to be referencing UIWebView as confirmed by the user at #854 (comment)

As the PayPal deprecated that SDK, and the sdk appears to be closed-source, the only solution is to move away from using the paypal sdk. Their deprecation notice can be found on their README, which contains this text:

Please use Braintree Direct in supported countries. In other countries, use Express Checkout and choose the Braintree SDK integration option.

So specifically, the solution will depend on your situation.

Hope this puts you on the right track, but I'll be locking this because this is not the place for support. If you have further questions on anything cordova related, please use our slack. For questions regarding the new way to use PayPal, you'll likely be better off going through their support channels.

@apache apache locked as resolved and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.