Refactor Auth social features and user credential creation #25
Conversation
| @@ -54,7 +55,7 @@ describe("#onAuthStateChange()", () => { | |||
| expect(listener).toHaveBeenCalledWith(credential.user); | |||
| }); | |||
|
|
|||
| it("doesn't invokes the listener after it's disposed", async () => { | |||
| @@ -26,11 +45,10 @@ export class User implements firebase.User, UserSchema { | |||
| password?: string; | |||
| phoneNumber: string | null; | |||
| photoURL: string | null; | |||
| providerData: (firebase.UserInfo | null)[]; | |||
There was a problem hiding this comment.
this was probably inherited from Firebase v4 or something.
| return Promise.resolve(new UserCredential(user, "signIn", isNewUser)); | ||
| } | ||
|
|
||
| private async signInWithSocial(provider: firebase.auth.AuthProvider) { |
There was a problem hiding this comment.
Maybe we use @firebase/auth-types to avoid having to use firebase.auth for every one of these types.
There was a problem hiding this comment.
This package is not intended for direct usage, and should only be used via the officially supported firebase package.
😕 from https://www.npmjs.com/package/@firebase/auth-types/v/0.10.1
There was a problem hiding this comment.
Yes, but that is simply because it is intended to be a private API for firebase, we are already very much working with internal APIs when mocking.
auth/auth.ts
Outdated
| const data = await this.store.consumeSocialMock(provider); | ||
| let user = this.store.findByProviderAndEmail(data.email, provider.providerId); | ||
| if (user) { | ||
| return this.signIn(user, { isNewUser: false }); | ||
| } | ||
|
|
||
| user = this.store.add({ ...data, providerId: provider.providerId }); | ||
| return this.signIn(user, { isNewUser: true }); |
There was a problem hiding this comment.
Little confused why it appears that we are removing the data from the store just to add it again
There was a problem hiding this comment.
Can you elaborate? I'm not following.
There was a problem hiding this comment.
Well if I am not mistaken consumeSocialMock() removes the provider from the store, but this.store.add() would add it back to the store.
Maybe consumeSocialMock() (and other functions) could use a JSDoc comment to elaborate on what it does to make this more clear?
There was a problem hiding this comment.
Added some comments around, let me know if it's clearer now!
| private idStore = new Map<string, UserSchema>(); | ||
| private emailStore = new Map<string, UserSchema>(); | ||
| private store = new Map<string, UserSchema>(); | ||
| private readonly socialMocks: SocialSignInMock[] = []; |
There was a problem hiding this comment.
Maybe it would make sense for this to be a Map or Set to simplify consumeSocialMock() with methods like .get(), .remove() or .has()?
auth/user-store.ts
Outdated
| findByEmail(email: string): User | undefined { | ||
| const schema = this.emailStore.get(email); | ||
| return schema ? new User(schema, this) : undefined; | ||
| for (const user of this.store.values()) { | ||
| if (user.email === email) { | ||
| return new User(user, this); | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems like it would be a good place for .find()
There was a problem hiding this comment.
I'm quite sure this made more sense when I wrote it 😅
There was a problem hiding this comment.
The reason why I wrote that that way is because .values() returns an iterator. To turn it into an array, one needs to spread it, which is an extra iteration if doing .find() on top of it.
Even though that's in theory slower, I went it to make that method shorter 👍
User#providerData;linkWithPopuphappen from theUserobject, so social mocks should be accessible by both it andAuth;UserCredentialcreation didn't seem very stable with 0ae4267, and to make matters worse there would be some duplication when implementingUser#linkWithPopup. This is now its own class which takes few and simple arguments.