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

fix(iOS): Making permissions switch statements exhaustive & supporting new iOS 14 cases #3160

Merged
merged 7 commits into from
Jul 1, 2020

Conversation

ikeith
Copy link
Contributor

@ikeith ikeith commented Jun 25, 2020

This PR supersedes #3135 and #3149. It unifies the handling of permission enums and fixes the compiler errors under Xcode 12 with the iOS 14 SDK.

#if swift(>=5.3)
case .limited:
// TODO: address this new case properly
#warning(".limited != .authorized, authorization status should be revisted for iOS 14")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to the new case, authorizationStatus() is being deprecated in favor of authorizationStatus(for:). The implications of .limited aren't fully clear to me yet so I'm adding this warning to be sure that it gets revisited before the beta period is over since it might require changes across more of Capacitor. But this addresses the immediate problem of compiles breaking with Xcode 12 until that time.

Co-authored-by: Dan Imhoff <dwieeb@gmail.com>
Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

I think the @unknown default cases should call reject with an error. In that case, if a new value comes out and we don't see it on time, when users get the error they will most likely report an issue about it that if they get "prompt" result.

ret = "denied"
case .authorized:
ret = "granted"
#if swift(>=5.3)
Copy link
Contributor

Choose a reason for hiding this comment

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

The new enum cases are not bound to Swift 5.3. They are added in iOS 14 and the availability check should check for os version instead.

Copy link
Member

Choose a reason for hiding this comment

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

As far as I know, they are only available on iOS 14 if you use SDK 14 to compile (Xcode 12), if you use SDK 13 (Xcode 11) then they won't be available because it works in a sort of "compatibility mode".

The #if swift(>=5.3) is only true on Xcode 12, it's false in Xcode 11 because it ships with swift 5.1 to 5.2.4.

I agree that using if #available(iOS 14, *) would be more clear, but sadly, it can't be used inside the switch.

Maybe is less confusing if we use #if compiler(>=5.3) instead of #if swift(>=5.3)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is true that language version is not synonymous with the SDK version but in practice it's not an issue. The only way it would be a problem is if you downloaded a newer version of the Swift compiler and replaced the toolchain in Xcode 11. If you did that, you should know what you're doing and can expect side effects in many places.

Not only is #available invalid inside a switch statement (so we'd need to have duplicated switches to use it), it doesn't actually solve our problem because that's a hook to insert a runtime check, meaning the new enum case would fail to compile under Xcode 11. There is no compiler flag that I'm aware of to strip code based on the SDK or OS version being targeted.

At this time, we need to be able to build cleanly with Xcode 11 and 12. Ultimately, this is only temporary for some months until 12 becomes the default.

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Code changes look good.

There is an user comment about the #if swift(>=5.3) that can be confusing, so maybe we can change it to #if compiler(>=5.3) if it makes it less confusing, or add a comment before the line explaining it, and maybe a TODO to remove that code once we drop support of Xcode 11 next year.

@ikeith ikeith merged commit f1d8c8c into master Jul 1, 2020
@ikeith ikeith deleted the permission-enums branch July 1, 2020 13:59
@buda2570
Copy link

@ikeith Thanks for the contribution to solve this problem of verisions, I have compiled my application in Capacitor again.

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.

5 participants