Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat($q): finally callback is called with result arguments #9287

Closed
wants to merge 1 commit into from
Closed

feat($q): finally callback is called with result arguments #9287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions src/ng/q.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,16 @@ function qFactory(nextTick, exceptionHandler) {
return this.then(null, callback);
},

always: function(callback) {
return this.then(function(value) {
safeInvoke(callback, value);
return value;
}, function(reason) {
safeInvoke(callback, reason);
return reject(reason);
});
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. an alternative implementation can be return this.then(callback, callback) but then the callback can control the reject/resolve chain, which is strange since the callback doesn't even know if this is a reject or resolve.
  2. we could allow this callback to return a promise in order to delay the resolve/reject of the chained promise, but still not effect the final result, similar to what finally does.

Copy link

Choose a reason for hiding this comment

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

(1) - it would make no sense to add 2 callbacks, it becomes "then"
(2) - I might misunderstand the code above, but that seems to be different from jQuery's implementation where the return of an always call is pushed to the next one in the queue (.then,
.always, etc) - in this case the return value is ignored and the chain continues with last known value; but I presume that is what you intended to 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.

so you are saying that you prefer the alternative implementation where:

always: function (callback) {
  return this.then(callback, callback);
});

I don't really care, but as I said, this would be a bit strange since the callback doesn't even know if this is a reject or resolve. I could do that just as easily, obviously, let's just try to agree on the implementation that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be ideal to behave the way other promise implementations do, I guess (in other words, I'll investigate the behaviour of other popular libraries tomorrow and leave comments based on that, unless someone else beats me to 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.

I'll have time to look into it tonight, so I guess you can do it too if you don't want to wait. From initial (and very superficial) look it doesn't seem like other promise implementations are doing something like this except for maybe jQuery.

Copy link

Choose a reason for hiding this comment

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

Yeah. Tried to search a bit but didn't find this "always" anywhere else.
Personally for me it works both ways. I was just saying that jQuery implemented "always()" in the same manner as "then" (stack-able via command-chain - even multiple always, as they say).

"finally": function(callback, progressBack) {
return this.then(function(value) {
return handleCallback(value, true, callback);
Expand Down Expand Up @@ -472,6 +482,14 @@ function qFactory(nextTick, exceptionHandler) {
}
};

var safeInvoke = function safeInvoke(callback, value) {
try {
callback(value);
} catch(e) {

}
};

/**
* @ngdoc method
* @name $q#when
Expand Down
65 changes: 65 additions & 0 deletions test/ng/qSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ describe('q', function() {
var promise = q(noop);
expect(typeof promise.then).toBe('function');
expect(typeof promise.catch).toBe('function');
expect(typeof promise.always).toBe('function');
expect(typeof promise.finally).toBe('function');
});

Expand Down Expand Up @@ -790,6 +791,40 @@ describe('q', function() {
expect(logStr()).toBe('error1(foo)->reject(foo); error2(foo)->reject(foo)');
});
});

describe('always', function() {
it('should be not effect reject reason', function() {
var promise = createPromise();
promise.always(error(1, 'bar')).then(null, error(2));
reject('foo');
mockNextTick.flush();
expect(logStr()).toBe('error1(foo)->bar; error2(foo)->reject(foo)');
});

it('should be not effect reject reason if exception thrown', function() {
var promise = createPromise();
promise.always(error(1, 'bar', true)).then(null, error(2));
reject('foo');
mockNextTick.flush();
expect(logStr()).toBe('error1(foo)->throw(bar); error2(foo)->reject(foo)');
});

it('should be not effect fulfillment value', function() {
var promise = createPromise();
promise.always(success(1, 'bar')).then(success(2));
resolve('foo');
mockNextTick.flush();
expect(logStr()).toBe('success1(foo)->bar; success2(foo)->foo');
});

it('should be not effect fulfillment value if exception thrown', function() {
var promise = createPromise();
promise.always(success(1, 'bar', true)).then(success(2));
resolve('foo');
mockNextTick.flush();
expect(logStr()).toBe('success1(foo)->throw(bar); success2(foo)->foo');
});
});
});
});

Expand Down Expand Up @@ -1132,6 +1167,10 @@ describe('q', function() {
expect(typeof promise['catch']).toBe('function');
});

it('should have an always method', function() {
expect(typeof promise.always).toBe('function');
});

it('should have a finally method', function() {
expect(typeof promise['finally']).toBe('function');
});
Expand Down Expand Up @@ -1535,6 +1574,32 @@ describe('q', function() {
expect(logStr()).toBe('error1(foo)->reject(foo); error2(foo)->reject(foo)');
});
});

describe('always', function() {
it('should be not effect reject reason', function() {
promise.always(error(1, 'bar')).then(null, error(2));
syncReject(deferred, 'foo');
expect(logStr()).toBe('error1(foo)->bar; error2(foo)->reject(foo)');
});

it('should be not effect reject reason if exception thrown', function() {
promise.always(error(1, 'bar', true)).then(null, error(2));
syncReject(deferred, 'foo');
expect(logStr()).toBe('error1(foo)->throw(bar); error2(foo)->reject(foo)');
});

it('should be not effect fulfillment value', function() {
promise.always(success(1, 'bar')).then(success(2));
syncResolve(deferred, 'foo');
expect(logStr()).toBe('success1(foo)->bar; success2(foo)->foo');
});

it('should be not effect fulfillment value if exception thrown', function() {
promise.always(success(1, 'bar', true)).then(success(2));
syncResolve(deferred, 'foo');
expect(logStr()).toBe('success1(foo)->throw(bar); success2(foo)->foo');
});
});
});
});

Expand Down