Skip to content

Conversation

davideast
Copy link
Collaborator

Checklist

Description

See #854 for the complete story and discussion. Comments here should strictly be for code review.

Code sample

// TODO:

/**
* Observable of authentication state
*/
authState: Observable<firebase.User>;
Copy link
Contributor

@cartant cartant Mar 13, 2017

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.

Copy link
Contributor

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.

Copy link
Collaborator Author

@davideast davideast Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cartant I'm not so sure, it is an observable of user. But I guess that confusion could be that it can be null, and the null value is useful itself. That makes authState more suitable. @jsayol mentioned using just .state(). Thoughts?

Copy link
Contributor

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.

@davideast davideast merged commit 0a8c8e3 into angular:master Mar 30, 2017
@davideast davideast deleted the feature-modules branch March 30, 2017 03:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants