feat($q): implement $q.race #14757
feat($q): implement $q.race #14757
Conversation
Implement $q.race. $q.race takes in an array or hash of promises and returns a promsie that is resolved as soon as the first promise is resolved. Closes angular#12929
| expect(logStr()).toBe('error(hi)->reject(hi)'); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
We should probably also test passing an object as the param, e.g. $q.race({a: promise1, b: promise2}).
There was a problem hiding this comment.
I've added tests for object params.
There was a problem hiding this comment.
Do we want to support this (considering it is not supported in native Promises)?
There was a problem hiding this comment.
Hmm, I am not too sure too. I decided to also take care of objects as this is also the case for $q.all.
|
Other than a few minor questions, this LGTM |
| * 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 |
There was a problem hiding this comment.
This description is a little missleading. It gives the impression we are looking for the first promise to actually resolve (not reject).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😇
| var deferred = defer(); | ||
|
|
||
| forEach(promises, function(promise) { | ||
| when(promise).then(deferred.resolve, deferred.reject); |
There was a problem hiding this comment.
@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().)
There was a problem hiding this comment.
I didn't realize you were using new Deferred() before. I think it's fine either way. Leaving it up to you!
|
LGTM - merging |
|
tweaked the docs description to match that on MDN thanks @yihangho |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Implement $q.race
What is the current behavior? (You can also link to an open issue here)
$q.race is non-existent (#12929)
Does this PR introduce a breaking change?
Nope.
Please check if the PR fulfills these requirements
Other information:
Implement $q.race. $q.race takes in an array or hash of promises and
returns a promsie that is resolved as soon as the first promise is
resolved.
Closes #12929