-
Notifications
You must be signed in to change notification settings - Fork 984
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!: Deprecate(/Remove) scrollView category extension on UIView #1400
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1400 +/- ##
=======================================
Coverage 78.31% 78.31%
=======================================
Files 16 16
Lines 1826 1826
=======================================
Hits 1430 1430
Misses 396 396 ☔ View full report in Codecov by Sentry. |
This causes issues for the statusbar plugin which relies on this to access the scroll view without knowing the specific type of the web view implementation. So putting this on hold for the moment. |
Currently this works because of a category extension that adds a `scrollView` method to every UIView instance, but that causes issues for SwiftUI so we want to remove that extension from cordova-ios. Since we do need to be able to look up the scrollView in this plugin, we can just define a private local method that does the same thing in a way that doesn't pollute global UIKit classes. Ref: apache/cordova-ios#1400
Currently this works because of a category extension that adds a `scrollView` method to every UIView instance, but that causes issues for SwiftUI so we want to remove that extension from cordova-ios. Since we do need to be able to look up the scrollView in this plugin, we can just define a private local method that does the same thing in a way that doesn't pollute global UIKit classes. Ref: apache/cordova-ios#1400
Currently this works because of a category extension that adds a `scrollView` method to every UIView instance, but that causes issues for SwiftUI so we want to remove that extension from cordova-ios. Since we do need to be able to look up the scrollView in this plugin, we can just define a private local method that does the same thing in a way that doesn't pollute global UIKit classes. Ref: apache/cordova-ios#1400
Currently this works because of a category extension that adds a `scrollView` method to every UIView instance, but that causes issues for SwiftUI so we want to remove that extension from cordova-ios. Since we do need to be able to look up the scrollView in this plugin, we can just define a private local method that does the same thing in a way that doesn't pollute global UIKit classes. Ref: apache/cordova-ios#1400
Status Bar plugin is fixed, but this API is used in a number of 3rd party plugins as well... I feel like our only option is going to be deprecation instead of removal (which doesn't actually address the issue that was raised about this) 😞 |
I don’t see what’s the problem with introducing a breaking change. |
f7d069d
to
2f3bf63
Compare
The best short term solution seems to be the following:
Re: breaking plugins due to moving the template to Swift, we tested and added compatible headers so that existing plugins should continue working even with the new template. It's possible we missed some edge cases, but we're definitely trying to keep existing plugins working (with lots of noisy deprecation warnings) |
This **removes** the API from Swift to solve the immediate problem in apache#1399 but leaves it available and deprecated in Objective-C due to use in 3rd party plugins. (There are only 2 Swift plugins that use this API as far as I can tell, and neither of them have very high usage or ongoing maintenance.) Closes apacheGH-1399.
2f3bf63
to
609170a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I tested and confirmed that Obj-C based plugins with self.webView.scrollView
will continue to work as expected and displays the deprecation warning.
Swift based plugins will need to update to continue working, which is expected.
var webViewScrollView: UIScrollView? {
let scrollViewSelector = NSSelectorFromString("scrollView")
if webView.responds(to: scrollViewSelector) {
return webView.perform(scrollViewSelector)?.takeUnretainedValue() as? UIScrollView
}
return nil
}
The equivalent Objective C code would be (copied from the statusbar plugin): - (UIScrollView *)webViewScrollView
{
SEL scrollViewSelector = NSSelectorFromString(@"scrollView");
if ([self.webView respondsToSelector:scrollViewSelector]) {
return ((id (*)(id, SEL))objc_msgSend)(self.webView, scrollViewSelector);
}
return nil;
} |
Platforms affected
iOS
Motivation and Context
As mentioned in #1399, this API was added as a compatibility shim at the time we supported both UIWebView and WKWebView, when WKWebView did not expose a public
scrollView
property. That property was made available in iOS 8, and this extension now risks causing problems due to being applied globally to all UIView subclasses.Closes #1399.
Description
Deprecate the category extension to expose a
scrollView
property on all UIView object instances, and remove it entirely from visibility to Swift.Testing
scrollView
propertyChecklist