search

matomo-org / matomo-sdk-ios

Matomo iOS, tvOS and macOS SDK: a Matomo tracker written in Swift
MIT License
388 stars 164 forks source link

Remove deprecated UIWebView #308

Closed goelay closed 5 years ago

goelay commented 5 years ago

ITMS-90809: Deprecated API Usage - Apple will stop accepting submissions of apps that use UIWebView APIs . See https://developer.apple.com/documentation/uikit/uiwebview for more information. After you’ve corrected the issues, you can use Xcode or Application Loader to upload a new binary to App Store Connect. Best regards, The App Store Team

Matamo is using UIWebView to get defaultuserAgent(). Please fix this.

brototyp commented 5 years ago

Hi @goelay, thanks for the bug report. Feel free to fix this. I am accepting PRs in this regard.

notjosh commented 5 years ago

An issue here is that WKWebView runs its JavaScript asynchronously.

Someone put together the methods of getting the User-Agent string here: https://gist.github.com/Koze/cfda5d2af12f6215424e.

End result is that URLSessionDispatcher.defaultUserAgent() can't return a String if it's using WKWebView :(

The good news is that the User-Agent is set async in the first place:

DispatchQueue.main.async {
    self.userAgent = userAgent ?? URLSessionDispatcher.defaultUserAgent()
}

But I imagine running on the JS thread would be a lot slowing in CPU-time, so there's probably greater risk of a race condition here if we use WKWebView than before, but what's the worst case scenario? An empty string? Is that acceptable? If so, I'm happy to make a PR for it.

brototyp commented 5 years ago

@notjosh Thanks for your ideas. I took the liberty and create a PR based on your ideas.

I do think it's not a huge issue if calculating the user agent takes a little longer. It's only needed at dispatch time. Events aren't set to the server right away but just once a few got collected. Event if there are quite some events collected right away, the user agent should already be generated at that time. Worst case is that some information that would usually be gathered from the user agent, isn't available on the backend.

If you don't mind, please have a look at my PR.

notjosh commented 5 years ago

Hey, awesome, this looks like it'll do the job just nicely. Great work :)

And, agree on the worst case being unlikely and not a huge issue. :shipit:

brototyp commented 5 years ago

Awesome. Thanks for the feedback everyone.This got merged and will be part of the next release.

brototyp commented 5 years ago

This just got released in version 7.0.0

xmanu commented 5 years ago

Thanks for the great work!

Could this possibly be backported to a 6.0.2 version that doesn't require iOS 10? I would like to keep iOS 9 as deployment target, but currently I would be forced to up that to 10, just because of MatomoTracker...

brototyp commented 5 years ago

Yeah, the problem is, that we currently can't run tests on Simulators smaller than iOS 10. Since we require Swift 5, there is (afaik) no way to execute a test on a iOS 9 Simulator while using Xcode 10.3, right?

xmanu commented 5 years ago

Yeah. That might be a problem... I will start a discussion about dropping iOS 9 with the team. This would also drop quite some legacy code in my case...

brototyp commented 5 years ago

The only thing I've got in mind on top of that is to fork, change it from iOS 10 to 9 in your fork and use it this way. It should work fine on iOS 9 as well. I didn't use any API that's not supported on iOS 9. It's just a bit of a workaround for now.