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

AngularFire 1.2 - firebaseRefProvider and $firebaseAuthService #703

Merged
merged 15 commits into from
Mar 22, 2016
Merged
12 changes: 12 additions & 0 deletions src/firebaseAuthService.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
(function() {
"use strict";

function FirebaseAuthService($firebaseAuth, firebaseRef) {
return $firebaseAuth(firebaseRef);
}
FirebaseAuthService.$inject = ['$firebaseAuth', 'firebaseRef'];
Copy link
Contributor

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])

Copy link
Contributor Author

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.


angular.module('firebase')
.factory('$firebaseAuthService', FirebaseAuthService);

})();
49 changes: 49 additions & 0 deletions src/firebaseRef.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
(function() {
"use strict";

function FirebaseRef() {
this.urls = null;
this.registerUrl = function registerUrl(urlOrConfig) {

if (typeof urlOrConfig === 'string') {
this.urls = {};
this.urls.default = urlOrConfig;
}

if (angular.isObject(urlOrConfig)) {
this.urls = urlOrConfig;
}

};

this.$$checkUrls = function $$checkUrls(urlConfig) {
if (!urlConfig) {
return new Error('No Firebase URL registered. Use firebaseRefProvider.registerUrl() in the config phase. This is required if you are using $firebaseAuthService.');
}
};

this.$$createRefsFromUrlConfig = function $$createMultipleRefs(urlConfig) {
var error = this.$$checkUrls(urlConfig);
if (error) { throw error; }
var defaultUrl = urlConfig.default;
var defaultRef = new Firebase(defaultUrl);
delete urlConfig.default;
angular.forEach(urlConfig, function(value, key) {
if (!defaultRef.hasOwnProperty(key)) {
defaultRef[key] = new Firebase(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Only methods directly declared on the Firebase ref will cause hasOwnProperty() to return true. It does not traverse the prototype chain:

Every object descended from Object inherits the hasOwnProperty method. This method can be used to determine whether an object has the specified property as a direct property of that object; unlike the in operator, this method does not check down the object's prototype chain. msdn

Thus, inherited or a non-enumerable property will return false and get overwritten. Overall, I'm not particularly happy with the idea of modifying the Firebase reference; other gotchas like this one may be waiting to trip us up.

Are there better alternatives? I know it's slightly less fun, but maybe just always placing the default at firebaseRef.default is the best answer? Perhaps just not using firebaseRef with multiple URLs is best? We can leave this up to the imagination of the small population of Edge Casians who will encounter it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reviewing it, I agree.

I think modifying the Firebase ref can open up a can of worms.

} else {
throw new Error(key + ' is a reserved property name on firebaseRef.');
}
});
return defaultRef;
};

this.$get = function FirebaseRef_$get() {
return this.$$createRefsFromUrlConfig(this.urls);
};
}

angular.module('firebase')
.provider('firebaseRef', FirebaseRef);
Copy link
Contributor

Choose a reason for hiding this comment

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

We've deviated here. All our other factories are prefixed with $, but firebaseRef is not. Unless you have a strong justification, let's be consistent because that's what we do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the impression that $ was for services that affected the $digest loop, where firebaseRef does not. I'm down with either I just want to clarify the meaning.

Copy link
Contributor

Choose a reason for hiding this comment

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

We used it for everything. I guess let's be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still like to see this changed. Do you feel strongly about not prefixing with $? All our other services follow this convention.


})();
27 changes: 27 additions & 0 deletions tests/unit/FirebaseAuthService.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';
describe('$firebaseAuthService', function () {
var firebaseRefProvider;
var MOCK_URL = 'https://stub.firebaseio-demo.com/'
Copy link
Contributor

Choose a reason for hiding this comment

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

I want const : )


beforeEach(module('firebase', function(_firebaseRefProvider_) {
firebaseRefProvider = _firebaseRefProvider_;
firebaseRefProvider.registerUrl(MOCK_URL);
}));

describe('<constructor>', function() {

var $firebaseAuthService;
beforeEach(function() {
module('firebase');
inject(function (_$firebaseAuthService_) {
$firebaseAuthService = _$firebaseAuthService_;
});
});

it('should exist based on using firebaseRefProvider.registerUrl()', inject(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does based on using mean here? Suggestion: "It should exist because we called firebaseRefProvider.registerUrl()"

expect($firebaseAuthService).not.toBe(null);
}));

});

});
56 changes: 56 additions & 0 deletions tests/unit/firebaseRef.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';
describe('firebaseRef', function () {

var firebaseRefProvider;
var MOCK_URL = 'https://stub.firebaseio-demo.com/'

beforeEach(module('firebase', function(_firebaseRefProvider_) {
firebaseRefProvider = _firebaseRefProvider_;
}));

describe('registerUrl', function() {

it('creates a single reference with a url', inject(function() {
firebaseRefProvider.registerUrl(MOCK_URL);
expect(firebaseRefProvider.$get()).toBeAFirebaseRef();
Copy link
Contributor

Choose a reason for hiding this comment

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

toBeAFirebaseRef() ☃☃☃

}));

it('creates a default reference with a config object', inject(function() {
firebaseRefProvider.registerUrl({
default: MOCK_URL
});
var firebaseRef = firebaseRefProvider.$get();
expect(firebaseRef).toBeAFirebaseRef();
}));

it('creates multiple references with a config object', inject(function() {
firebaseRefProvider.registerUrl({
default: MOCK_URL,
messages: MOCK_URL + 'messages'
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah the endless debate about whether we should suffix the root URL or concat all child paths with a separator. Reminds me of my old PHP days and little beauties like this: join(DIRECTORY_SEPARATOR, func_get_args($segments));

There's nothing to see here. Move along.

});
var firebaseRef = firebaseRefProvider.$get();
expect(firebaseRef).toBeAFirebaseRef();
expect(firebaseRef.messages).toBeAFirebaseRef();
}));

it('should throw an error when no url is provided', inject(function () {
function errorWrapper() {
firebaseRefProvider.registerUrl();
firebaseRefProvider.$get();
}
expect(errorWrapper).toThrow();
}));

it('should throw an error when a reserved property is used', inject(function() {
function errorWrapper() {
firebaseRefProvider.registerUrl({
path: 'hello'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see a MOCK_URL replace 'hello'; I know this is nitpicky, but we're not checking what gets thrown, and if the URI is invalid and that causes a throw somewhere, we could end up with a false positive.

});
firebaseRefProvider.$get();
}
expect(errorWrapper).toThrow();
}));

});

});