-
Notifications
You must be signed in to change notification settings - Fork 139
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
MSAL: Update ios min version to ios 14.0 and macos to 10.13 #1594
Conversation
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.
Please update MSAL.podspec and Package.swift targets as well. Code changes look good otherwise.
Done |
if (error) *error = msidError; | ||
return NO; | ||
} | ||
NSError *msidError = MSIDCreateError(MSIDErrorDomain, MSIDErrorInvalidDeveloperParameter, @"parentViewController is a required parameter on iOS 13.", nil, nil, nil, nil, nil, YES); |
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.
Shouldn't this error say ... required parameter on iOS 14
since that's the required minimum now?
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.
you are right, thanks . I have it fixed in this PR : #1630
Is there any reason why iOS 13 could not be supported? I don't see anything specific in the changes |
@@ -350,7 +350,7 @@ - (void)allAccountsFromDevice:(MSALAccountEnumerationParameters *)parameters | |||
{ | |||
BOOL shouldCallBroker = NO; | |||
|
|||
if (@available(iOS 13.0, macOS 10.15, *)) | |||
if (@available(macOS 10.15, *)) |
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.
Shouldn't this be if (@available(iOS 14.0, macOS 10.15, *))
else the code inside the condition would not compile for iOS anymore? The code would only run on macOS since iOS was omitted
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.
i believe the "*" indicates that the code is also for all other platforms beyond the ones listed . So it should be available for iOS.
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.
@Veena11 of course! Thanks :) Although is there API required that's no longer going to work on iOS 13? Xcode 14 seems to be otherwise okay with iOS 13 as a minimum target
we intend to support last 3 versions (16, 15, 14) of iOS. |
Proposed changes
Update ios min version to ios 14.0 and macos to 10.13
Type of change
Risk
Additional information