-
Notifications
You must be signed in to change notification settings - Fork 1
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
User linking #27
User linking #27
Conversation
@@ -43,12 +43,9 @@ export class MockAuth implements firebase.auth.Auth { | |||
return this.signIn(user, { isNewUser: true }); | |||
} | |||
|
|||
fetchProvidersForEmail(email: string): Promise<any> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
likely a relic from Firebase v4 or something.
I'll give this a review on Friday. I'm traveling right now 😅 I look forward to trying it out when I'm back. |
Bold! 😷 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't noticed any logical errors, I am a little bit wary of the optional chaining after a function call but I will leave it up to you if it's something that is worth refactoring findByEmail
for.
auth/auth.ts
Outdated
fetchSignInMethodsForEmail(email: string): Promise<string[]> { | ||
throw new Error("Method not implemented."); | ||
const providers = this.store.findByEmail(email)?.providerData || []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having findByEmail()
return undefined seems like a possible places that we could clean up.
If all other usages I see of findByEmail()
we are throwing errors, maybe findByEmail()
should just throw an error itself?
Does fetchSignInMethodsForEmail()
in Firebase throw when you give it an invalid email?
Either way I am not a big fan of optional chaining directly after a function call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
fetchSignInMethodsForEmail()
in Firebase throw when you give it an invalid email?
It doesn't.
Either way I am not a big fan of optional chaining directly after a function call.
Can fix! 😃
auth/auth.test.ts
Outdated
describe("#fetchSignInMethodsForEmail()", () => { | ||
it("returns list of sign in methods", async () => { | ||
const auth = new MockAuth(app); | ||
await auth.createUserWithEmailAndPassword("foo@bar.com", "baz"); | ||
|
||
// sign in with a different method, to make sure things don't mix up | ||
const provider = new firebase.auth.FacebookAuthProvider(); | ||
auth.mockSocialSignIn(provider).respondWithUser("John", "john@doe.com"); | ||
auth.signInWithPopup(provider); | ||
|
||
return expect(auth.fetchSignInMethodsForEmail("foo@bar.com")).resolves.toEqual([ | ||
firebase.auth.EmailAuthProvider.PROVIDER_ID, | ||
]); | ||
}); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add in test for cases where the email has not been configured?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup done
edf0d6d
to
2034c89
Compare
Closes #26.
Builds on top of #25.