-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat(module): WIP Release Candidate API #855
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
Conversation
/** | ||
* Observable of authentication state | ||
*/ | ||
authState: Observable<firebase.User>; |
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.
Would you consider renaming the authState
property to user
? Because that's what it is. If the underlying SDK entity is a firebase.User
, how likely is it that authState
will ever be anything other than a user? And if one of the goals of the RC is to co-exist with the SDK, shouldn't the two use the same terminology?
Having to use auth
(in the beta) to refer to what is the user is a peeve of mine and I've seen the confusion that it causes in SO questions. If it has to be explained on SO, surely it's a problem.
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.
Hmm, I should have waited until I'd had my morning coffee before posting the above comment. With FirebaseAuthState
now gone, using authState
as the name of the observable property seems fine. Please ignore my pre-coffee review.
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.
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.
authState
is totally fine. As per my second comment, I wasn't paying close enough attention when I made the first. authState
fits well with the SDK's onAuthStateChanged
- which calls a function, passing the user. It's all good. I would have deleted my original comment had I been able to.
Checklist
npm install
,npm run build
, andnpm test
run successfully? YesDescription
See #854 for the complete story and discussion. Comments here should strictly be for code review.
Code sample
// TODO: