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
Show file tree
Hide file tree
Changes from 1 commit
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
29 changes: 29 additions & 0 deletions src/ng/q.js
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,34 @@ 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 = new Deferred();

forEach(promises, function(promise) {
when(promise).then(function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a situation where forEach can throw? If there is then we should catch it and reject the deferred.

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, interesting. I am not too sure, but looking at the source for forEach, I don't think it will throw.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed the code inside the forEach callback (when(...).then(...)) won't throw.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, this can be abbreviated as:

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.

Indeed the code inside the forEach callback (when(...).then(...)) won't throw.

I think what @petebacondarwin meant is more on whether forEach will throw on its own (i.e., due to bad input), no? But as mentioned, looking at how forEach is implemented, I don't think that will happen. Furthermore, since when(...).then(...) is also safe, I guess this should be safe. 😄

BTW, this can be abbreviated as:

when(promise).then(deferred.resolve, deferred.reject);

Good point. Thanks for pointing this out!

deferred.resolve(value);
}, function(reason) {
deferred.reject(reason);
});
});

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 +693,7 @@ function qFactory(nextTick, exceptionHandler, errorOnUnhandledRejections) {
$Q.when = when;
$Q.resolve = resolve;
$Q.all = all;
$Q.race = race;

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

describe('race', 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('exception logging', function() {
var mockExceptionLogger = {
log: [],
Expand Down