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

Cordova iOS 8 proposal #113

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Cordova iOS 8 proposal #113

wants to merge 1 commit into from

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Jun 11, 2024

Please view the proposal on the Files tab and leave comments there 🙂

@breautek
Copy link
Contributor

breautek commented Jun 11, 2024

Just as a data point...

Google Maps uses a min deployment target of iOS 15 14, and even support for that is "frozen", e.g. they aren't addressing any issues with it, and will be removing support in next/future major release. They also require XCode 15 when consuming their framework

I'm not saying we should go to 15 or anything, but requiring iOS 14+ for WKScriptMessageHandlerWithReply I don't think is a problem. But maintaining backwards compatibility with existing plugins that uses the older API is a larger issue as you've noted. So for that I think the question is whether we want to maintain 2 cordova API entry points.

@jcesarmobile
Copy link
Member

jcesarmobile commented Jun 11, 2024

Requiring xcode 15 is not a problem, we already have a PR for it
apache/cordova-ios#1425

The dependency of Cordova plugins to the Cordova-ios SPM is only needed if we want the plugin to build independently without adding it to an app.
I removed it from my WIP PR
apache/cordova-plugin-device#196

At the moment plugins don’t build independently neither, so it’s not a big deal to continue like that

@dpogue
Copy link
Member Author

dpogue commented Jun 12, 2024

I'm not saying we should go to 15 or anything, but requiring iOS 14+ for WKScriptMessageHandlerWithReply I don't think is a problem. But maintaining backwards compatibility with existing plugins that uses the older API is a larger issue as you've noted. So for that I think the question is whether we want to maintain 2 cordova API entry points.

I need to look at this more closely and try to figure out exactly how we would implement it as part of our CDVWebViewEngine and CDVCommandQueue stuff.

I do not want (any more) WebKit-specific classes getting exposed publicly in Cordova API, because I'm certain that will come back to bite us at some point the way having UIWebView stuff did. We have the engine abstraction layer, we should be strict about maintaining that boundary.

@jcesarmobile
Copy link
Member

A problem with moving to Swift templates is that it will break all push plugins since they rely on an Objective-C category on the AppDelegate for adding the method that gets the token from APNS.

Example:
https://github.com/uniquare/cordova-plugin-push/blob/master/src/ios/AppDelegate%2Bnotification.m

It won't work if there is no AppDelegate.h since needs to import it
https://github.com/uniquare/cordova-plugin-push/blob/master/src/ios/AppDelegate%2Bnotification.h#L9

Not sure if there is an alternative for getting the token from Swift. I tried with Swift extensions for Capacitor and didn't work, but could be related to the extension being on the plugins, which are CocoaPods libraries, might work if the extension is part of the app as it will be in a Cordova app.

@dpogue
Copy link
Member Author

dpogue commented Jun 12, 2024

It should still be possible to use Objective-C categories to add the methods for handling push token events, but it would need to extend the CDVAppDelegate base class rather than the AppDelegate at the app level.
Example:
https://github.com/AyogoHealth/cordova-plugin-ayogo-push/blob/master/src/ios/CDVAppDelegate%2BPush.h
https://github.com/AyogoHealth/cordova-plugin-ayogo-push/blob/master/src/ios/CDVAppDelegate%2BPush.m

I remember trying to do this sort of thing with Swift extensions and it not working, and it seemed to be due to some sort of load ordering thing where the extension wasn't getting applied automatically.

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.

3 participants