Skip to content

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

Merged
merged 3 commits into from
Dec 18, 2017
Merged

Conversation

yramanchuk
Copy link
Contributor

New FUIAuth sign in callback returns FIRAuthDataResult

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:")));
Copy link
Contributor

Choose a reason for hiding this comment

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

add "instead"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

if (error) {
[self invokeResultCallbackWithUser:user error:error];
[self invokeResultCallbackWithAuthDataResult:authResult error:error];
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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];
Copy link
Contributor

Choose a reason for hiding this comment

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

please fix indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@morganchen12 morganchen12 left a 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];
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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."
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@yramanchuk yramanchuk merged commit a69aa95 into master Dec 18, 2017
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.

3 participants