Skip to content

watchOS support for FirebaseAuth #5585

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

Closed
wants to merge 15 commits into from
Closed

watchOS support for FirebaseAuth #5585

wants to merge 15 commits into from

Conversation

ryanwilson
Copy link
Member

@ryanwilson ryanwilson commented May 12, 2020

This is a re-implementation of #4632 on top of master, along with some small improvements in code structure that were needed after some refactoring.

Credit goes to @hohteri for doing this work in the first place!

Fix #4621

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@paulb777
Copy link
Member

In this PR or after, please restore the FirebaseStorage watchOS pod lib lint tests removed in #5643 to workaround CocoaPods not being able to handle a test only dependency on FirebaseAuth for linting.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@ryanwilson
Copy link
Member Author

@hohteri apologies for the trouble, but do you mind commenting here with only "@googlebot I consent." in order to appease the one true googlebot and get the CLA flag working? Hoping to land this very soon.

@hohteri
Copy link
Contributor

hohteri commented Jun 11, 2020

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@ryanwilson ryanwilson marked this pull request as ready for review June 11, 2020 20:39
Tested with a small sample app in SwiftUI with anonymous sign in,
       email/password sign up and email/password log in.
Copy link
Contributor

@maksymmalyhin maksymmalyhin left a comment

Choose a reason for hiding this comment

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

Great!

- (FIRUser *)getStoredUserForAccessGroup:(NSString *)accessGroup
projectIdentifier:(NSString *)projectIdentifier
error:(NSError *_Nullable *_Nullable)outError;
- (FIRUser *_Nullable)getStoredUserForAccessGroup:(NSString *)accessGroup
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: - (nullable FIRUser *)... looks more usual to me but I don't mind leaving it as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Originally had it to match the above function, but I'll change them both to nullable.

@@ -47,10 +48,12 @@ supports email and password accounts, as well as several 3rd party authenticatio
s.ios.framework = 'SafariServices'
s.dependency 'FirebaseCore', '~> 6.8'
s.dependency 'GoogleUtilities/AppDelegateSwizzler', '~> 6.5'
s.dependency 'GoogleUtilities/Environment', '~> 6.5'
s.dependency 'GoogleUtilities/Environment', '~> 6.6'
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder is there an issue with bumping GoogleUtilities version in this PR as well? Since we use local specs in the tests it should not break anything.

Would you like to update GoogleUtilities changelog in the PR as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yep GoogleUtilities should bump to 6.7 here and in its podspec for the new API

@@ -47,10 +48,12 @@ supports email and password accounts, as well as several 3rd party authenticatio
s.ios.framework = 'SafariServices'
s.dependency 'FirebaseCore', '~> 6.8'
s.dependency 'GoogleUtilities/AppDelegateSwizzler', '~> 6.5'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think, versions of all GoogleUtilities dependencies should be updated

@@ -1,3 +1,6 @@
# Unreleased
Copy link
Member

Choose a reason for hiding this comment

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

Will be 6.7.0 for sure

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed offline - will leave it as is until it's released.

@@ -56,12 +60,21 @@ typedef void (^FIRAuthAPNSTokenCallback)(FIRAuthAPNSToken *_Nullable token,
*/
- (instancetype)init NS_UNAVAILABLE;

/** @fn initWithApplication:bundle
#if TARGET_OS_WATCH
Copy link
Member

Choose a reason for hiding this comment

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

Something like GDTCORPlatform to abstract WKExtension and UIApplication might be cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out these weren't actually needed - only for Phone Auth which was disabled already 😅 removed. Thanks for making me look a second time!

ryanwilson added a commit that referenced this pull request Aug 12, 2020
This is a redo of #5585, which was a re-implementation of #4632. This
ignores any changes to GoogleUtilities, those will be done in a later
PR to create a common KeyedArchiver utility.

Co-authored-by: Harri Hohteri <[email protected]>
@ryanwilson
Copy link
Member Author

Closing in favour of the more up to date #6260

@ryanwilson ryanwilson closed this Aug 12, 2020
ryanwilson added a commit that referenced this pull request Aug 12, 2020
* watchOS support for FirebaseAuth.

This is a redo of #5585, which was a re-implementation of #4632. This
ignores any changes to GoogleUtilities, those will be done in a later
PR to create a common KeyedArchiver utility.

Co-authored-by: Harri Hohteri <[email protected]>

* Fixed lint errors, added GHA tests.

* Enabled GHA for PRs as well.

Co-authored-by: Harri Hohteri <[email protected]>
@firebase firebase locked and limited conversation to collaborators Sep 12, 2020
@paulb777 paulb777 deleted the rw-auth-watchos branch January 10, 2021 00:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Firebase Auth support for watchOS
6 participants