Skip to content

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.

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