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

feat($q): implement $q.race #14757

Closed
wants to merge 3 commits into from
Closed

feat($q): implement $q.race #14757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fac6030
feat($q): implement $q.race
yihangho Jun 13, 2016
1bf49e3
test($q): add test for $q.race taking in objects
yihangho Jun 15, 2016
dc2be34
refactor($q): Simplify $q.race
yihangho Jun 16, 2016
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
25 changes: 25 additions & 0 deletions src/ng/q.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,30 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
return deferred.promise;
}

/**
* @ngdoc method
* @name $q#race
* @kind function
*
* @description
* Returns a promise that is resolved similarly as the first promise to resolve.
*
* @param {Array.<Promise>|Object.<Promise>} promises An array or hash of promises.
* @returns {Promise} Returns a promise that will be resolved as soon as the first promise in
Copy link
Member

Choose a reason for hiding this comment

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

This description is a little missleading. It gives the impression we are looking for the first promise to actually resolve (not reject).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about something like this:

Returns a promise that will be settled as soon as the first promise in `promises` is settled. The value or reason will be the same as that of the first settled promise.

Note that the term settled is also used in the spec.

Copy link
Member

Choose a reason for hiding this comment

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

I just realized that we are using resolved inconsistently in the docs (either as fulfilled or resolved in the sense of the spec) 😒 (TBH, I myself thought resolved means fulfilled until now...)

I would suggest being a little more explicit:

Returns a promise that will be settled (i.e. fulfilled or rejected)...

Don't forget to also update the description above 😇

* `promises` is resolved. The resolved value will be the same as the value of the first
* resolved promise.
*/

function race(promises) {
var deferred = defer();

forEach(promises, function(promise) {
when(promise).then(deferred.resolve, deferred.reject);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gkalpak Does this look good to you? (Notice I changed the initialization of deferred from new Deferred() to defer(). The reason is the the resolve and reject methods are bounded if the object is coming from defer().)

Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize you were using new Deferred() before. I think it's fine either way. Leaving it up to you!

});

return deferred.promise;
}

var $Q = function Q(resolver) {
if (!isFunction(resolver)) {
throw $qMinErr('norslvr', "Expected resolverFn, got '{0}'", resolver);
Expand Down Expand Up @@ -665,6 +689,7 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
$Q.when = when;
$Q.resolve = resolve;
$Q.all = all;
$Q.race = race;

return $Q;
}
72 changes: 72 additions & 0 deletions test/ng/qSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1986,6 +1986,78 @@ describe('q', function() {
});
});

describe('race (array)', function() {
it('should do nothing if given an empty array', function() {
q.race([]).then(success(), error());
expect(mockNextTick.queue.length).toBe(0);
expect(logStr()).toBe('');
});

it('should resolve as soon as the first promise is settled by resolution', function() {
var deferred1 = defer(),
deferred2 = defer();

q.race([promise, deferred1.promise, deferred2.promise]).then(success(), error());
expect(logStr()).toBe('');
syncResolve(deferred1, 'hi');
expect(logStr()).toBe('success(hi)->hi');
syncResolve(deferred2, 'cau');
expect(logStr()).toBe('success(hi)->hi');
syncReject(deferred, 'hola');
expect(logStr()).toBe('success(hi)->hi');
});

it('should reject as soon as the first promise is settled by rejection', function() {
var deferred1 = defer(),
deferred2 = defer();

q.race([promise, deferred1.promise, deferred2.promise]).then(success(), error());
expect(logStr()).toBe('');
syncReject(deferred1, 'hi');
expect(logStr()).toBe('error(hi)->reject(hi)');
syncResolve(deferred2, 'cau');
expect(logStr()).toBe('error(hi)->reject(hi)');
syncReject(deferred, 'hola');
expect(logStr()).toBe('error(hi)->reject(hi)');
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably also test passing an object as the param, e.g. $q.race({a: promise1, b: promise2}).

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've added tests for object params.

Copy link
Member

Choose a reason for hiding this comment

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

Do we want to support this (considering it is not supported in native Promises)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I am not too sure too. I decided to also take care of objects as this is also the case for $q.all.

describe('race (hash)', function() {
it('should do nothing if given an empty array', function() {
q.race({}).then(success(), error());
expect(mockNextTick.queue.length).toBe(0);
expect(logStr()).toBe('');
});

it('should resolve as soon as the first promise is settled by resolution', function() {
var deferred1 = defer(),
deferred2 = defer();

q.race({a: promise, b: deferred1.promise, c: deferred2.promise}).then(success(), error());
expect(logStr()).toBe('');
syncResolve(deferred1, 'hi');
expect(logStr()).toBe('success(hi)->hi');
syncResolve(deferred2, 'cau');
expect(logStr()).toBe('success(hi)->hi');
syncReject(deferred, 'hola');
expect(logStr()).toBe('success(hi)->hi');
});

it('should reject as soon as the first promise is settled by rejection', function() {
var deferred1 = defer(),
deferred2 = defer();

q.race({a: promise, b: deferred1.promise, c: deferred2.promise}).then(success(), error());
expect(logStr()).toBe('');
syncReject(deferred1, 'hi');
expect(logStr()).toBe('error(hi)->reject(hi)');
syncResolve(deferred2, 'cau');
expect(logStr()).toBe('error(hi)->reject(hi)');
syncReject(deferred, 'hola');
expect(logStr()).toBe('error(hi)->reject(hi)');
});
});

describe('exception logging', function() {
var mockExceptionLogger = {
log: [],
Expand Down