-
Notifications
You must be signed in to change notification settings - Fork 632
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
AngularFire 1.2 - firebaseRefProvider and $firebaseAuthService #703
Conversation
I'm not going to be able to give this PR the time and consideration it deservers. Leaving this for review by someone else. |
Overall, this looks solid. How will queries and order by criteria work here? It looks like I would use firebaseRef.messages.orderByChild(...) and similar? I feel like more thought should be given to these and whether they need any special consideration. |
function FirebaseAuthService($firebaseAuth, firebaseRef) { | ||
return $firebaseAuth(firebaseRef); | ||
} | ||
FirebaseAuthService.$inject = ['$firebaseAuth', 'firebaseRef']; |
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.
We don't use $inject
anywhere else. Note that I don't dislike this approach and I even find it more readable, but it's inconsistent, and if we're going to change the strategy, we should do that in a sep changelist.
The current format is: angular.module('firebase').factory('$firebaseAuthService', ["$firebaseAuth", "firebaseRef", FirebaseAuthService])
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.
This is the proposed structure in the Angular Styleguide, which is supported by the core team.
I would like for us to move that way eventually. I think starting here is fine because of how small and isolated it is from the rest of the codebase.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Thanks, David! LGTM with one exception (firebaseRef vs $firebaseRef). Please look over the comments and we'll get this merged! |
Addresses proposal #684.
cc: @jamestalmage, @katowulf, @jwngr
Introduces two new services:
firebaseRefProvider
and$firebaseAuthService
.firebaseRefProvider
The
firebaseRefProvider
makes sure you never have to writenew Firebase("...")
ever again. This makes dependency injection with Firebase references much easier.Multiple urls are also supported, but a default must be provided, and you cannot use a Firebase Ref property like
path
orthen
.Then in a controller/service/factory you can inject the
firebaseRef
:$firebaseAuthService
Once you have configured a
default
reference usingfirebaseRefProvider
, the$firebaseAuthService
will work as a automatically configured$firebaseAuth
instance.