-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fixed use of Q#any and added test. #838
base: master
Are you sure you want to change the base?
Conversation
That test failure is due to |
Same issue here for my /pull/841 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m very sorry this has fallen to the bottom of our community’s inbox. This is a good change and I’m sure one of us would be glad to spin up a release.
We would like an entry in the changelog. A nice follow-up for the community would be a change to use keepachangelog.org format!
If someone would not mind following up on this, please do not block on my word. I’ve been leaning pretty hard on focusing my free time on game projects.
@@ -1665,7 +1665,9 @@ function any(promises) { | |||
} | |||
|
|||
Promise.prototype.any = function () { | |||
return any(this); | |||
return this.then(function(values) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be merely this.then(any)
.
It also looks like CI has gone stale because we’re testing on a version of Node.js that does not support the syntax used by one of our testing libraries. Wouldn’t it be nice if people used semver to indicate when they’re using new JavaScript syntax that their users might not support ;-) Regardless, the fix is to move up the water line in .travis.yml |
Q#any
(.any
on a promise instance) previously would never resolve due toany
requiring its parameter to be an array.