feat(authentication): add Sign-In Anonymously

[trancee]

Pull request checklist

Please check if your PR fulfills the following requirements:

• The changes have been tested successfully.
• A changeset has been created (npm run changeset).

Close #35

[trancee]

@robingenz What's the idea behind linkWithGoogle or linkWithXXX? I know there is a method called linkWithCredential but it requires a AuthCredential from Firebase which we do not have.

Did you think something like this, similar to the signInWithGoogle method but then use the result for linkWithCredential?

  public async linkWithGoogle(
    options?: SignInOptions,
  ): Promise<SignInResult> {
    const provider = new GoogleAuthProvider();
    this.applySignInOptions(options || {}, provider);
    const auth = getAuth();
    const userCredential = await signInWithPopup(auth, provider);
    const authCredential =
      GoogleAuthProvider.credentialFromResult(userCredential);
    const user = await linkWithCredential(auth.currentUser, authCredential)
    return this.createSignInResult(user, authCredential);
  }

[robingenz]

@trancee This would be my first attempt:

public async linkWithGoogle(): Promise<SignInResult> {
    const auth = getAuth();
    if (!auth.currentUser) {
        throw new Error(FirebaseAuthenticationWeb.ERROR_NO_USER_SIGNED_IN);
    }
    const provider = new GoogleAuthProvider();
    const userCredential = await linkWithPopup(auth.currentUser, provider)
    const authCredential = GoogleAuthProvider.credentialFromResult(userCredential);
    return this.createSignInResult(userCredential, authCredential);
}

The user is not provided. The operation always affects the current user.
Options are not needed, because the Firebase JS SDK only needs user and auth provider (see here).

[trancee]

@robingenz There are 3 ways to linking:

• linkWithCredential
• linkWithPopup
• linkWithRedirect

We are mainly using linkWithPopup and not linkWithRedirect. Or should there be an option to use either of them?
I am using linkWithPhoneNumber when linking the phone number (currently not implemented). And linkWithCredential is being used when linking email link.

By the way: I have added SignInMethod as a way to avoid duplicating the provider names, I hope that's fine.

[robingenz]

We are mainly using linkWithPopup and not linkWithRedirect. Or should there be an option to use either of them?

No, we currently only support popups.

I am using linkWithPhoneNumber when linking the phone number (currently not implemented).

👍

And linkWithCredential is being used when linking email link.

👍

By the way: I have added SignInMethod as a way to avoid duplicating the provider names, I hope that's fine.

Good idea! But i suggest to call it SignInProvider (better fits to the config option providers) and to not export it (or is this necessary?). You can also remove the comments. They are not very helpful from my pov.

[trancee]

Good idea! But i suggest to call it SignInProvider (better fits to the config option providers) and to not export it (or is this necessary?). You can also remove the comments. They are not very helpful from my pov.

I had name it the same way Google already named it in their docs:
https://firebase.google.com/docs/reference/js/auth.md#signinmethod

However, they also seem to mix it between providerId and signInMethod
https://firebase.google.com/docs/reference/js/auth.authcredential#properties

And I just noticed they also have this for providerId:
https://firebase.google.com/docs/reference/js/auth.md#providerid

Should we do the same, or keep it as SignInProvider or better rename to Provider or something?

[robingenz]

Oh, looks like there is no wrong or right. 😅 I leave it up to you. We can keep it that way for now if you want.

[trancee]

I have added the linkWithProvider method to simplify the usage of different providers. This is just a suggestion, it would remove a lot of code duplication for basically the same thing. This could also be applied to the signInWith... methods. What do you think?

[robingenz]

I have added the linkWithProvider method to simplify the usage of different providers. This is just a suggestion, it would remove a lot of code duplication for basically the same thing. This could also be applied to the signInWith... methods. What do you think?

To be honest, I don't like that approach for 3 reasons:

1. I prefer several smaller methods than one big method, even if you have less duplicate code. From my point of view, it is more understandable and therefore better to maintain (especially for new users). But that is of course a matter of taste!
2. You have many parameters, which are optional, because they may be needed only for a particular provider. This also means that you cannot force certain parameters when choosing a provider.
3. I want to keep it equal to the signIn methods which I will not remove until Capacitor 6 as it would be a breaking change.

[trancee]

To be honest, I don't like that approach for 3 reasons:

1. I prefer several smaller methods than one big method, even if you have less duplicate code. From my point of view, it is more understandable and therefore better to maintain (especially for new users). But that is of course a matter of taste!
2. You have many parameters, which are optional, because they may be needed only for a particular provider. This also means that you cannot force certain parameters when choosing a provider.
3. I want to keep it equal to the signIn methods which I will not remove until Capacitor 6 as it would be a breaking change.

Thanks for sharing your opinion on this. I guess there are always pros/cons and I understand your point. I will keep single methods for each provider.

[trancee]

If the definitions are fine with you, I will start with the implementation.

[robingenz]

LGTM. You can start with the implementation.

[robingenz]

Thank you! I will review again these days. I just had little time.

[robingenz]

@trancee Sorry this whole process is taking so long because of me. I'm unfortunately sick at the moment and rarely online. I will review as soon as possible.

[trancee]

@trancee Sorry this whole process is taking so long because of me. I'm unfortunately sick at the moment and rarely online. I will review as soon as possible.

No worries. Take your time to recover, this can wait.

[robingenz]

@trancee I made a few changes on your branch. I hope that was ok. Otherwise we can revert it. I want to get the next release out in a timely manner and had a few more comments.
I would like you to review my changes and help me test them.

[robingenz]

npm i @capacitor-firebase/authentication@1.0.0-dev.3000cf3.1664034972

[trancee]

@trancee I made a few changes on your branch. I hope that was ok. Otherwise we can revert it. I want to get the next release out in a timely manner and had a few more comments. I would like you to review my changes and help me test them.

I had to revert one change where there is a special circumstance. When you link an account, there are ways to use a directly linking method like linkWithProvider which then does not require another linkWithCredental in handleSuccessfulLink otherwise it will result in an error. So I have reverted that and tested it and it works in my test cases.

I also had to add the providerID for PASSWORD in order to be able to unlink from that provider again.

By the way, I was not able to test Google Play Games as I am not able to set it up properly on my side.

[robingenz]

@trancee Looks great! Thank you so much for your work!

[robingenz]

@trancee Can we remove the SignInMethod enum? Seems like it is not used anywhere.

[trancee]

@trancee Can we remove the SignInMethod enum? Seems like it is not used anywhere.

Yes, I will remove this since it is not being used anywhere.

[robingenz]

@trancee Great! Thank you so much! I will merge this today.