-
Notifications
You must be signed in to change notification settings - Fork 483
Implement additional FUIAuth sign-in call bac #375
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
Conversation
FirebaseAuthUI/FUIAuth.h
Outdated
error:(nullable NSError *)error; | ||
error:(nullable NSError *)error | ||
__attribute__((deprecated("This is deprecated API and will be removed in a future release." | ||
"Use authUI:didSignInWithAuthDataResult:error:"))); |
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.
add "instead"
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.
Done
FirebaseAuthUI/FUIAuth.m
Outdated
} | ||
|
||
if (error) { | ||
[self invokeResultCallbackWithUser:user error:error]; | ||
[self invokeResultCallbackWithAuthDataResult:authResult error:error]; |
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.
If an error exist we shouldn't pass authResult here which should ideally be nil, but at this level we do not know.
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.
Done
[self showAlertWithMessage:message]; | ||
}); | ||
}]; | ||
// The dispatch is a workaround for a bug in FirebaseAuth 3.0.2, which doesn't call the |
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.
This bug has been fixed, there is no need to dispatch async to the main queue here again anymore.
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.
Done
} | ||
|
||
NSString *message = | ||
[NSString stringWithFormat:FUILocalizedString(kStr_PasswordRecoveryEmailSentMessage), email]; |
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 fix indentation.
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.
Done
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 bump the FirebaseAuth dependency in the podfile, otherwise LGTM.
// The dispatch is a workaround for a bug in FirebaseAuth 3.0.2, which doesn't call the | ||
// completion block on the main queue. | ||
dispatch_async(dispatch_get_main_queue(), ^{ | ||
[self.delegate decrementActivity]; |
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.
If we're removing this, we should bump the Podfile dependency to 3.1 or higher so this bug doesn't regress for users who haven't updated their pods.
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.
we already have 4.2 as minimum requirement https://github.com/firebase/FirebaseUI-iOS/blob/master/FirebaseUI.podspec#L50
FirebaseAuthUI/FUIAuth.h
Outdated
@param user The signed in user if the sign in attempt was successful. | ||
@param error The error that occurred during sign in, if any. | ||
*/ | ||
- (void)authUI:(FUIAuth *)authUI | ||
didSignInWithUser:(nullable FIRUser *)user | ||
error:(nullable NSError *)error; | ||
error:(nullable NSError *)error | ||
__attribute__((deprecated("This is deprecated API and will be removed in a future release." |
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 can remove the first line of the deprecation message, since Xcode already adds a "deprecated" label to the warning that's generated.
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.
Done
New FUIAuth sign in callback returns FIRAuthDataResult