Fix auth bug #98
Fix auth bug #98
Conversation
|
@chrisbianca I have fixed |
|
@chrisbianca any update on this ? |
There was a problem hiding this comment.
@lorstenoplo thank you for the submission and apologies for the delay in reviewing this properly. I've added a few suggested changes to the PR, which I'm very happy to make myself if you don't have the time, but wanted to highlight the suggestions before doing that. They're only minor and just to ensure consistency across the hooks.
Let me know either way and we can get this merged and released.
|
|
||
| export default ( | ||
| auth: firebase.auth.Auth, | ||
| email: string, |
There was a problem hiding this comment.
I think it would be better to make email and password variables that are passed into the returned login function. This is more in keeping with what some of the other libraries that allow mutation of data, e.g. apollo
There was a problem hiding this comment.
Actually I was thinking about the exact same thing
|
|
||
| export default ( | ||
| auth: firebase.auth.Auth, | ||
| email: string, |
There was a problem hiding this comment.
As above, I think it would be better to make email and password variables that are passed into the returned register function
| }); | ||
| }; | ||
|
|
||
| const resArray: loginHook = [loggedInUser, error, login, loading]; |
There was a problem hiding this comment.
From a return perspective, I think that the function should be returned as the first item in the array and the order of the other elements should match the order of useAuthState.
So it should be: [login, loggedInUser, loading, error]
| }); | ||
| }; | ||
|
|
||
| const resArray: registerHook = [registeredUser, error, register, loading]; |
There was a problem hiding this comment.
As above, the ordering should match what's proposed for login
|
The thing is I have no time ( exams ). I could have made those changes but I don't want to make mistakes ( as I have exams = tension ) |
|
No problem at all. I'll get this merged and then make the changes. Thanks again for the PR and good luck with your exams! |
|
@lorstenoplo just FYI, I also updated the names of the hooks to better match the functionality that they were implementing. This means that we could add wider support for underlying authentication methods in the future. This has now been released in https://github.com/CSFrequency/react-firebase-hooks/releases/tag/v3.0.1 |
Added two new hooks for sign In and Register
loading,credential,errorparams providedShould Close: #85 , close: #46 , close: #43