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

feat: Add compile-time decision for disabling UIWebView #584

Merged
merged 1 commit into from
Jan 4, 2020

Conversation

beevelop
Copy link
Contributor

This PR fixes #583.

With the introduction of the WK_WEB_VIEW_ONLY flag, references to UIWebView are removed from the source. I tried to adapt the same changes from apache/cordova-ios#715 to this project.

@ath0mas
Copy link

ath0mas commented Nov 28, 2019

Tried your fork, with and without "WKWebViewOnly" preference, and it's OK for me
💯

** BUILD SUCCEEDED **

@beevelop you didn't change references in plugin.xml as you suggested? and indeed I think it would prevent plugin to install or build correctly when not using "WKWebViewOnly" preference?

@beevelop
Copy link
Contributor Author

Thank you for the feedback.

Yeah, modifying the plugin.xml is not necessary when using the compile-time approach. This way, we also stay backwards compatible.

@ath0mas
Copy link

ath0mas commented Dec 2, 2019

Please, any update from team to review/merge this? And possible release 😇 ?

@mjamal4u
Copy link

mjamal4u commented Dec 3, 2019

Please, any update from team to review/merge this? And possible release 😇 ?

We are also in same pool. Would be great help if we can have early release :)

@bbialas
Copy link

bbialas commented Dec 10, 2019

@beevelop any chances to merge this one soon?

@beevelop
Copy link
Contributor Author

@bbialas Unfortunately, I'm not in charge.
Maybe @breautek, @janpio or @NiklasMerz can help with that?

@NiklasMerz
Copy link
Member

I am not an Apache Commiter, too. Since I still got no warning from Apple after uploading an app I did not test the flag, yet.

@dpa99c You are an expert for in app browser.

@breautek
Copy link
Contributor

Sorry @beevelop I generally stay away from iOS pull requests because I don't have personal apple hardware to use for testing, and I'm not very experienced in objective-c either.

But i can say that PRs won't get merged into until all checks passes. Currently it is failing for what appears to be an issue with the tests getting a webview context.

No webview context. Retries remaining: 2
cordova-paramedic: Tests failed to complete; ending session. The error is:
Error: [context("WEBVIEW_5906.1")] Error response status: 21, Timeout - An operation did not complete before its timeout expired. Selenium error: Automation Server Error -- Selenium didn't complete your last command on time.
For help, please check https://wiki.saucelabs.com/display/DOCS/Common+Error+Messages

https://travis-ci.org/apache/cordova-plugin-inappbrowser/jobs/617811060?utm_medium=notification&utm_source=github_status

My educated guess tells me this is because the tests are probably trying to run in a cordova-ios environment without a wkwebview plugin installed. But not 100% sure if this is the case.

@beevelop
Copy link
Contributor Author

@breautek The builds are failing due to an error in the pipelines. They are not exactly related to the changes in this PR. See #491 or compare to #528 (CI checks also failed). Not quite sure, on how to proceed there.

@ath0mas
Copy link

ath0mas commented Dec 11, 2019

@breautek how to/can we rerequest check suite?

@Kepro
Copy link

Kepro commented Dec 18, 2019

c'mon guys! you can do it! cannot wait for fix :) like really cannot wait :D

@Kepro
Copy link

Kepro commented Dec 19, 2019

hey, I tried these changes locally but now I got CDVPlugin class CDVUIInAppBrowser (pluginName: uiinappbrowser) does not exist.

@ath0mas
Copy link

ath0mas commented Dec 20, 2019

@Kepro more context or log or a minimal reproducible example is needed to understand your problem

@Kepro
Copy link

Kepro commented Dec 20, 2019

@ath0mas hi there, sorry!
"cordova-ios": "5.1.1" (latest)
cordova-plugin-inappbrowser: 3.1.0 (latest)

2019-12-20 09:36:38.164820+0100 XXX532:103173] Apache Cordova native platform version 5.1.1 is starting.
2019-12-20 09:36:38.167505+0100 XXX[532:103173] Multi-tasking -> Device: YES, App: YES
2019-12-20 09:36:38.184551+0100 XXX[532:103173] [CDVTimer][console] 0.204921ms
2019-12-20 09:36:38.185113+0100 XXX[532:103173] [CDVTimer][handleopenurl] 0.293016ms
2019-12-20 09:36:38.193607+0100 XXX[532:103173] [CDVTimer][intentandnavigationfilter] 8.319974ms
2019-12-20 09:36:38.194196+0100 XXX[532:103173] [CDVTimer][gesturehandler] 0.292063ms
2019-12-20 09:36:38.227851+0100 XXX[532:103173] [CDVTimer][file] 33.380032ms
2019-12-20 09:36:38.229283+0100 XXX[532:103173] [CDVTimer][inappbrowser] 1.068950ms
2019-12-20 09:36:38.229725+0100 XXX[532:103173] CDVPlugin class CDVUIInAppBrowser (pluginName: uiinappbrowser) does not exist.
2019-12-20 09:36:38.229895+0100 XXX[532:103173] [CDVTimer][uiinappbrowser] 0.292897ms
2019-12-20 09:36:38.230252+0100 XXX[532:103173] [CDVTimer][wkinappbrowser] 0.262022ms
2019-12-20 09:36:38.246181+0100 XXX[532:103173] [CDVTimer][splashscreen] 15.802026ms
2019-12-20 09:36:38.262580+0100 XXX[532:103173] [CDVTimer][statusbar] 16.147971ms
2019-12-20 09:36:38.264041+0100 XXX[532:103173] [CDVTimer][localstorage] 1.145959ms
2019-12-20 09:36:38.264271+0100 XXX[532:103173] [CDVTimer][TotalPluginStartup] 80.597043ms

@Kepro
Copy link

Kepro commented Dec 20, 2019

okay, it now complains about it but it's working together with cordova-plugin-wkwebview-engine in config.xml

			<!-- Cordova WKWebView engine  -->
			<preference name="WKWebViewOnly" value="true" />
			<preference name="CordovaWebViewEngine" value="CDVWKWebViewEngine" />
			<feature name="CDVWKWebViewEngine">
				<param name="ios-package" value="CDVWKWebViewEngine" />
			</feature>

debug

2019-12-20 12:19:12.953981+0100 xxx[874:136760] Using WKWebView
2019-12-20 12:19:12.955626+0100 xxx874:136760] [CDVTimer][console] 0.215054ms
2019-12-20 12:19:12.956149+0100 xxx[874:136760] [CDVTimer][handleopenurl] 0.254035ms
2019-12-20 12:19:12.964714+0100 xxx[874:136760] [CDVTimer][intentandnavigationfilter] 8.396983ms
2019-12-20 12:19:12.965167+0100 xxx[874:136760] [CDVTimer][gesturehandler] 0.229955ms
2019-12-20 12:19:12.970830+0100 xxx[874:136760] [CDVTimer][file] 5.452037ms
2019-12-20 12:19:12.971308+0100 xxx[874:136760] [CDVTimer][inappbrowser] 0.223994ms
2019-12-20 12:19:12.971499+0100 xxx[874:136760] CDVPlugin class CDVUIInAppBrowser (pluginName: uiinappbrowser) does not exist.
2019-12-20 12:19:12.971592+0100 xxx[874:136760] [CDVTimer][uiinappbrowser] 0.149012ms
2019-12-20 12:19:12.971875+0100 xxx[874:136760] [CDVTimer][wkinappbrowser] 0.194073ms

@ath0mas
Copy link

ath0mas commented Dec 20, 2019

Not much time to look at it these days so if anyone has a feedback for @Kepro feel free :)

@breautek breautek mentioned this pull request Dec 22, 2019
3 tasks
@chukwumaokere
Copy link

chukwumaokere commented Dec 22, 2019

Hi, Sorry to be "that guy" but I'm fairly new at this. I have encountered this issue and I want to use the PR version that has this fix for a client production build I'm working on, but I'm not sure how to specify in some file to pull the version from this PR instead of the regular dependency that's lacking this fix. Any tips?

Thanks in advance!

@breautek
Copy link
Contributor

Hi, Sorry to be "that guy" but I'm fairly new at this. I have encountered this issue and I want to use the PR version that has this fix for a client production build I'm working on, but I'm not sure how to specify in some file to pull the version from this PR instead of the regular dependency that's lacking this fix. Any tips?

Thanks in advance!

No problem, there is a first time for everything! I'll provide a step-by-step instructions below.

  1. Fork the inappbrowser plugin to your own account using the "Fork" button on github, on the top-right of the page.
  2. Run git clone <your fork repo url>
  3. Now we need to add the repo we want to merge in form as a new remote. In this case, from neohelden's github. So run git remote add neohelden https://github.com/neohelden/cordova-plugin-inappbrowser.git
  4. Now we can fetch neohelden's changesets. Run git fetch neohelden
  5. Because neohelden made their changes in their master branch, we can merge their master branch into your own fork. Run git merge neohelden/master
  6. Now push the changes your own repo. Run git push
  7. Finally, you can install your version of the plugin by using, cordova plugin add https://yourgithubforkurl

Alternatively, you can just install neohelden's copy by using cordova plugin add https://github.com/neohelden/cordova-plugin-inappbrowser.git which might be a good idea just to test it out before spending the time preparing your own fork, but you're at the mercy of a third-party making unexpected changes, or even deleting the fork/repo altogether, so it's just safer to create your own fork to ensure unexpected changes.

@chukwumaokere
Copy link

Worked perfectly. You've been a tremendous help, @breautek
Cant thank you enough!

@ath0mas
Copy link

ath0mas commented Dec 22, 2019

@beevelop were you able to look at the log @Kepro gets at runtime?
CDVPlugin class CDVUIInAppBrowser (pluginName: uiinappbrowser) does not exist.

@erisu erisu added this to the 3.2.0 milestone Dec 24, 2019
@rajashekaranugu
Copy link

Hello All, do we have any update regarding this issue?. Thanks in advance

@NiklasMerz
Copy link
Member

NiklasMerz commented Dec 31, 2019 via email

@timbru31
Copy link
Member

timbru31 commented Jan 7, 2020

@irene-pc - yes, the new release is still in the voting phase and the 48 hours have not yet passed.

See Niklas' second link for the current voting situation (and feel free to test the release and add a non-binding vote)

@irene-pc
Copy link

irene-pc commented Jan 7, 2020

@irene-pc - yes, the new release is still in the voting phase and the 48 hours have not yet passed.

See Niklas' second link for the current voting situation (and feel free to test the release and add a non-binding vote)

Thank you so much for the information @timbru31 I'm newbie here and I don't know very well how does updates work and when are they available.

@Kepro
Copy link

Kepro commented Jan 8, 2020

if I use "master" branch and I will use
then I got
2020-01-08 17:01:09.517925+0100 InSight Assess[2441:1535723] [CDVTimer][inappbrowser] 0.198960ms 2020-01-08 17:01:09.518268+0100 InSight Assess[2441:1535723] CDVPlugin class CDVUIInAppBrowser (pluginName: uiinappbrowser) does not exist. 2020-01-08 17:01:09.518376+0100 InSight Assess[2441:1535723] [CDVTimer][uiinappbrowser] 0.271916ms 2020-01-08 17:01:09.518652+0100 InSight Assess[2441:1535723] [CDVTimer][wkinappbrowser] 0.187993ms

nobody else got this issue?

@brodycj
Copy link

brodycj commented Jan 8, 2020

In case of issues with the master branch or upcoming release from npm then please raise a new issue with a minimal, reproducible example. We do not have extra resources to support discussions in merged PRs or closed issues.

@irene-pc
Copy link

irene-pc commented Jan 8, 2020

Does anyone know when version 3.2.0 will be available in npm?

@NiklasMerz
Copy link
Member

NiklasMerz commented Jan 8, 2020 via email

@brodycj
Copy link

brodycj commented Jan 8, 2020

FYI we have finished voting on the release and expect to publish it very soon. Please feel free to ping me personally (don't ping any other individuals) in case it really does take too long.

Release discussions and voting threads generally happen on the mailing list. It is also fine to contact us by Slack in case of any questions. Links to find the mailing list and Slack channel are in the footer of cordova.io which redirects to: cordova.apache.org

In general, we would appreciate a reasonable amount of patience since Apache Cordova is maintained by a bunch of generally overworked volunteers. But please do feel free to follow up with us in case of any unreasonable delays.

@mjamal4u
Copy link

mjamal4u commented Jan 8, 2020

For all incredible contributors on this issue. This release means a lot for our Apps. Thank you !!!

@NiklasMerz
Copy link
Member

Thank you all here for your patience and testing this PR. I am proud to announce that version 3.2.0 is now available on NPM. The release blog post will be on cordova.apache.org very soon.

@irene-pc
Copy link

irene-pc commented Jan 9, 2020

Thank you so much to all of you.

@brodybits thank you so much for your response. I am newbie in the whole process of launching a new version :)

@NiklasMerz
Copy link
Member

This has been answered many times. Please read above. The release is in progress. Please be patient. Asking won't make it faster. Releases are available when they are done.

@irene-pc Revisiting this comment now, I realised that it could sound rude. That was not my intent and I am sorry about that.

I am new member of the PMC and I am still figuring out how to work on releases. Chris and Tim did a great job to explain how the Cordova community works. I would advise you to join the mailing list to get the latest news.

Thank you all in this thread very much for supporting my first release.

@ath0mas
Copy link

ath0mas commented Jan 10, 2020

Released 3.2.0 works perfect, thanks you to all contributors 😍
And congrats @NiklasMerz for your first release! :)

@irene-pc
Copy link

Hallo @NiklasMerz Nothing happens. It is normal. It's your first release and maybe it is all new for you. You did a great job. My main concern was Apple email and with this update I don't receive it. Thank you so much 👍
I think it is a great idea to join the mailing list. How can I do that?

@NiklasMerz
Copy link
Member

@irene-pc You can find everything here https://cordova.apache.org/contact/

@irene-pc
Copy link

@NiklasMerz Thank you

@jtibbles
Copy link

Thanks @NiklasMerz . Quick question - does the plugin need the platform ios5.1.1 to work or is 5.1.0 sufficient?

@NiklasMerz
Copy link
Member

This should work with 5.1.0 where WKWebViewOnly was introduced. But version 5.1.1 is just a patch version with just a few fixes you should prefer .

@erisu
Copy link
Member

erisu commented Jan 16, 2020

@jtibbles @NiklasMerz I would recommend using Cordova-iOS 5.1.1.

In 5.1.0, yes the WKWebViewOnly flag was introduced but there was a bug that caused the cordova platform add ios command to fail when the flag was set in config.xml before adding the platform. See issue ticket for more details: apache/cordova-ios#725

This bug is fixed and released in Cordova-iOS 5.1.1.

@timbru31
Copy link
Member

timbru31 commented May 7, 2020

Please seek support on StackOverflow or in our Slack community.

@n-elhk
Copy link

n-elhk commented May 11, 2020

Hi everyone,
I have the same problem as here: #584 (comment) .

package.json:

"cordova-plugin-ionic-webview": "^4.2.1",
"cordova-plugin-inappbrowser": "^3.2.0",
"cordova-ios": "^5.1.1",

config. xml:
preference name="WKWebViewOnly" value="true"

My error: CDVPlugin class CDVUIInAppBrowser (pluginName: uiinappbrowser) does not exist.

@Yelles
Copy link

Yelles commented May 19, 2020

Hi everyone,
I have the same problem as here: #584 (comment) .

package.json:

"cordova-plugin-ionic-webview": "^4.2.1",
"cordova-plugin-inappbrowser": "^3.2.0",
"cordova-ios": "^5.1.1",

config. xml:
preference name="WKWebViewOnly" value="true"

My error: CDVPlugin class CDVUIInAppBrowser (pluginName: uiinappbrowser) does not exist.

same for me, any solution ?

@NiklasMerz
Copy link
Member

Please try deleting the iOS platform and add it again. That should clean stuff up that got lost in the update. Make sure you are using the latest versions of those plugins and platforms.

Locking this because pull requests and issues are not for questions. As Tim said please go to Stack Overflow or Slack.

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

Successfully merging this pull request may close these issues.

iOS - build fails if using "WKWebViewOnly" preference