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

Es6 style promises #531

Merged
merged 4 commits into from
Jan 21, 2015
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions src/FirebaseAuth.js
Original file line number Diff line number Diff line change
Expand Up @@ -222,24 +222,27 @@
*/
_routerMethodOnAuthPromise: function(rejectIfAuthDataIsNull) {
var ref = this._ref;
var deferred = this._q.defer();

function callback(authData) {
if (authData !== null) {
deferred.resolve(authData);
} else if (rejectIfAuthDataIsNull) {
deferred.reject("AUTH_REQUIRED");
} else {
deferred.resolve(null);
return this._utils.promise(function(resolve,reject){
function callback(authData) {
// Turn off this onAuth() callback since we just needed to get the authentication data once.
ref.offAuth(callback);

if (authData !== null) {
resolve(authData);
Copy link

Choose a reason for hiding this comment

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

I have found it to be good practice to add a return statement before every call to resolve() or reject() to make sure no code gets run after them. I've had too many bugs where I didn't do this and they are a huge pain to track down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I agree with your premise. Angular promises are now A++ compliant. Part of that spec states that once a promise is resolved or rejected, it's state is fixed permanently - future calls to resolve or reject will not change the promises state. Once you call resolve(A), you can call resolve(B) or reject(E) as often as you like - the promise will always be resolved with A.

Are you possibly thinking about always returning a value from a promise handler? That makes a lot of sense. It allows chaining.

That said - I am totally willing to change it.

Copy link

Choose a reason for hiding this comment

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

No, I have more pedantic reasons for wanting the return in there. I'm not worried about the promise getting resolved or rejected multiple times since, as you said, the A++ spec prevents that. But I do think in most cases it is unexpected and unnecessary to run more code after the promise is resolved or rejected. Putting an explicit return in there will stop future lines in that function from running, which is what we want in most cases. Not doing so can lead to some nasty bugs where code runs when don't expect it to.

But yes, allowing for chaining is also a positive, so let's do it! But when you do add the return statements, you'll need to move the ref.offAuth(callback) above the if block or else the return statements will exit early causing that code to not run.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chaining isn't at issue here (we aren't in then callback).

I made the changes requested. IntelliJ complains that the return statements are unnecessary, but the linter passes.

return;
Copy link

Choose a reason for hiding this comment

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

Hmmm, not quite what I was envisioning. I'd just do return resolve(authData) and return reject("AUTH_REQUIRED"), etc. No need for it to be on a separate line. And then you can enable the chaining as you mentioned.

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 think you might be conflating these methods with $q.resolve and $q.reject, which return promises. These methods behave like deferred.resolve and deferred.reject which do not have specified return values. The original code provides chain-able promises.

See the $q constructor documentation, they aren't returning there at all. I would think it's bad practice to return the result of a function whose return value is NOT(edit) guaranteed by documentation / specification (they may be leaving it's return value unspecified intentionally to allow for future changes).

Copy link

Choose a reason for hiding this comment

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

Ahh okay, okay, I definitely mis-spoke there and you are correct about the return value/chaining thing. I still prefer stylistically to have the return on the same line as the resolve / reject (and this is actually what we do with a lot of our backend Node.js code), but I'm not going to block this PR any further on that.

}
else if (rejectIfAuthDataIsNull) {
reject("AUTH_REQUIRED");
return;
}
else {
resolve(null);
Copy link

Choose a reason for hiding this comment

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

Should be return resolve(null).

Copy link

Choose a reason for hiding this comment

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

This guys is still missing a return.

}
}

// Turn off this onAuth() callback since we just needed to get the authentication data once.
ref.offAuth(callback);
}

ref.onAuth(callback);

return deferred.promise;
ref.onAuth(callback);
});
},

/**
Expand Down
22 changes: 12 additions & 10 deletions src/firebase.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,16 +128,18 @@
}
applyLocally = !!applyLocally;

var def = $firebaseUtils.defer();
ref.transaction(valueFn, function(err, committed, snap) {
if( err ) {
def.reject(err);
}
else {
def.resolve(committed? snap : null);
}
}, applyLocally);
return def.promise;
return new $firebaseUtils.promise(function(resolve,reject){
ref.transaction(valueFn, function(err, committed, snap) {
if( err ) {
reject(err);
Copy link

Choose a reason for hiding this comment

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

Please add return statements in this method as well.

Copy link

Choose a reason for hiding this comment

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

Please just put them on the same line. Thanks!

return;
}
else {
resolve(committed? snap : null);
return;
}
}, applyLocally);
});
},

$asObject: function () {
Expand Down
42 changes: 29 additions & 13 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,29 @@

.factory('$firebaseUtils', ["$q", "$timeout", "firebaseBatchDelay",
function($q, $timeout, firebaseBatchDelay) {

// ES6 style promises polyfill for angular 1.2.x
// Copied from angular 1.3.x implementation: https://github.com/angular/angular.js/blob/v1.3.5/src/ng/q.js#L539
function Q(resolver) {
if (!angular.isFunction(resolver)) {
throw new Error('missing resolver function');
}

var deferred = $q.defer();

function resolveFn(value) {
deferred.resolve(value);
}

function rejectFn(reason) {
deferred.reject(reason);
}

resolver(resolveFn, rejectFn);

return deferred.promise;
}

var utils = {
/**
* Returns a function which, each time it is invoked, will pause for `wait`
Expand Down Expand Up @@ -220,21 +243,14 @@
});
},

defer: function() {
return $q.defer();
},
defer: $q.defer,

reject: function(msg) {
var def = utils.defer();
def.reject(msg);
return def.promise;
},
reject: $q.reject,

resolve: function() {
var def = utils.defer();
def.resolve.apply(def, arguments);
return def.promise;
},
resolve: $q.when,

//TODO: Remove false branch and use only angular implementation when we drop angular 1.2.x support.
promise: angular.isFunction($q) ? $q : Q,

makeNodeResolver:function(deferred){
return function(err,result){
Expand Down
64 changes: 64 additions & 0 deletions tests/unit/utils.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -254,3 +254,67 @@ describe('$firebaseUtils', function () {
});

});

describe('#promise (ES6 Polyfill)', function(){

var status, result, reason, $utils, $timeout;

function wrapPromise(promise){
promise.then(function(_result){
status = 'resolved';
result = _result;
},function(_reason){
status = 'rejected';
reason = _reason;
});
}

beforeEach(function(){
status = 'pending';
result = null;
reason = null;
});

beforeEach(module('firebase',function($provide){
$provide.decorator('$q',function($delegate){
//Forces polyfil even if we are testing against angular 1.3.x
return {
defer:$delegate.defer,
all:$delegate.all
}
});
}));

beforeEach(inject(function(_$firebaseUtils_, _$timeout_){
$utils = _$firebaseUtils_;
$timeout = _$timeout_;
}));

it('throws an error if not called with a function',function(){
expect(function(){
$utils.promise();
}).toThrow();
expect(function(){
$utils.promise({});
}).toThrow();
});

it('calling resolve will resolve the promise with the provided result',function(){
wrapPromise(new $utils.promise(function(resolve,reject){
resolve('foo');
}));
$timeout.flush();
expect(status).toBe('resolved');
expect(result).toBe('foo');
});

it('calling reject will reject the promise with the provided reason',function(){
wrapPromise(new $utils.promise(function(resolve,reject){
reject('bar');
}));
$timeout.flush();
expect(status).toBe('rejected');
expect(reason).toBe('bar');
});

});