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

(ios): rename CDVWKProcessPoolFactory #825

Merged
merged 2 commits into from
Jan 7, 2021

Conversation

NiklasMerz
Copy link
Member

@NiklasMerz NiklasMerz commented Nov 25, 2020

Platforms affected

iOS

Motivation and Context

CDVWKProcessPoolFactory was integrated from the WKWebView plugin with
the new name: CDVWebViewProcessPoolFactory

Description

This fixes the import of the processpool factory for cordova-ios versions 6 or greater. The process pool is now part of cordova-ios and needs to be available public see: apache/cordova-ios#1031

Closes #751

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

CDVWKProcessPoolFactory was integrated from the WKWebView plugin with
the new name: CDVWebViewProcessPoolFactory
src/ios/CDVWKInAppBrowser.m Outdated Show resolved Hide resolved
@NiklasMerz NiklasMerz force-pushed the processpool branch 2 times, most recently from e0bdf02 to 68a0a80 Compare November 26, 2020 07:31
Copy link
Member

@timbru31 timbru31 left a comment

Choose a reason for hiding this comment

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

LGTM, but obviously only if we merge in apache/cordova-ios#1031 :)

@timbru31
Copy link
Member

timbru31 commented Jan 6, 2021

With Darryl's suggestion the milestone could be moved to a minor release I guess?

@NiklasMerz
Copy link
Member Author

Yes the change is not breaking anymore. Looks like the next release will be major anyways and we don't have a minor milestone.

@NiklasMerz NiklasMerz merged commit de31942 into apache:master Jan 7, 2021
@jcesarmobile
Copy link
Member

since 4.1.0 introduced breaking changes maybe we should do a minor release first that fixed that problem and then we can continue with the breaking changes.
But that would mean we need to rever the merged breaking changes to be able to do a minor release

or we can continue with the major release as not supporting Xcode 10 is not a big issue since its builds are not accepted in the App Store

@NiklasMerz
Copy link
Member Author

Yeah I was just thinking about when and how to release next. Maybe we should do a discuss thread on the list.

Personally I think I would go for a major release now. I would agree that supporting Xcode 10 should not be that important since Xcode 11 is required for the app store and the current release is even 12. We should reduce our workload and move forward with the limited time we have.

@jcesarmobile Do you know issues or users about Xcode 10?

@jcesarmobile
Copy link
Member

@jcesarmobile Do you know issues or users about Xcode 10?

Don't understand the question, the issue on Xcode 10 is linked on my previous comment #831

The easy fix for users is to use Xcode 11 or newer to compile, if we wanted to support it a #if __IPHONE_OS_VERSION_MAX_ALLOWED >= 130000 macro should be used, but probably not worth it

@NiklasMerz
Copy link
Member Author

I was just asking if you know about people having issues with this version. I personally always prioritize work if users are complaining and it's not just something that came up internally.

@jcesarmobile
Copy link
Member

I created the issue and nobody has added the 👍 reaction to it, so I guess nobody was affected by it.

@NiklasMerz
Copy link
Member Author

As this fix needs this PR to be released with cordova-ios apache/cordova-ios#1031 I think we could wait for that or are there reasons to release inappbrowser now as 5.0.0? The milestone looks good.

NiklasMerz referenced this pull request in apache/cordova-ios Feb 9, 2021
* set CDVWebViewProcessPoolFactory to public

* Move CDVWebViewProcessPoolFactory.m to Classes/Public
@nmanikiran
Copy link

Hi @NiklasMerz
i am facing issues with cookie sync between wkwebview & InappBrowser even after updating to v5.0.0
for more i logged an issue

FE-Roger pushed a commit to hogangnono/cordova-plugin-inappbrowser that referenced this pull request Sep 27, 2021
* (ios): rename CDVWKProcessPoolFactory

CDVWKProcessPoolFactory was integrated from the WKWebView plugin with
the new name: CDVWebViewProcessPoolFactory

* (ios): Allow both processpool imports
jessyefuster pushed a commit to jessyefuster/cordova-plugin-inappbrowser that referenced this pull request Aug 16, 2023
* (ios): rename CDVWKProcessPoolFactory

CDVWKProcessPoolFactory was integrated from the WKWebView plugin with
the new name: CDVWebViewProcessPoolFactory

* (ios): Allow both processpool imports
pesi-hgc pushed a commit to pesi-hgc/cordova-plugin-inappbrowser-transparent that referenced this pull request Nov 3, 2023
* (ios): rename CDVWKProcessPoolFactory

CDVWKProcessPoolFactory was integrated from the WKWebView plugin with
the new name: CDVWebViewProcessPoolFactory

* (ios): Allow both processpool imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WKWebview cookie sometimes can't be get after upgrade to Cordova 6.1.0
6 participants