Skip to content
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

fix(modules): Separate AngularFire services into modules #841

Merged
merged 7 commits into from
Aug 16, 2016

Conversation

abeisgoat
Copy link
Contributor

Modularizes internal shenanigans

@coveralls
Copy link

coveralls commented Aug 16, 2016

Coverage Status

Coverage decreased (-4.4%) to 90.0% when pulling 0cd6413 on modulefire into 8a32f0c on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.4%) to 90.0% when pulling d399c05 on modulefire into 8a32f0c on master.

@davideast davideast changed the title Modulefire fix(modules): Separate AngularFire services into modules Aug 16, 2016
@davideast
Copy link
Contributor

LGTM!

@davideast davideast merged commit c4127e6 into master Aug 16, 2016
@davideast davideast deleted the modulefire branch August 16, 2016 19:33
@abeisgoat
Copy link
Contributor Author

🐐

angular.module("firebase", ["firebase.utils", "firebase.config", "firebase.auth", "firebase.database"])
//TODO: use $window
.value("Firebase", exports.firebase)
Copy link

Choose a reason for hiding this comment

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

Why are we still exporting Firebase with a capital "F"? This should not be required anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we removed this it would be a breaking change which we're avoiding here.

Copy link

Choose a reason for hiding this comment

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

Makes sense. Let's at least add a TODO comment here to remove this line when the 3.0.0 release happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants