Change the type signature on Q.resolve, and add Promise.prototype.delay #35
Conversation
| */ | ||
| function delay(delayMs, returnVal) { | ||
| function delay(delayMsOrVal, opt_delayMs) { |
There was a problem hiding this comment.
This is a pretty major change to the behavior of delay(). Should that be documented somewhere in case it breaks something for someone?
There was a problem hiding this comment.
this is for consistency with Q. We added delay pretty recently, and none of the existing callers were using the second arg.
There was a problem hiding this comment.
(though i generally agree that when we bump the version, we should do a
minor version change instead of a micro version change)
On Wed, Mar 26, 2014 at 11:24 AM, Sho Kuwamoto [email protected]:
In kew.js:
*/
-function delay(delayMs, returnVal) {
+function delay(delayMsOrVal, opt_delayMs) {This is a pretty major change to the behavior of delay(). Should that be
documented somewhere in case it breaks something for someone?Reply to this email directly or view it on GitHubhttps://github.com//pull/35/files#r10981131
.
There was a problem hiding this comment.
I was specifically concerned about folks we may not know about who may be relying on existing behavior (probably a very small number at this point, but still...). If something breaks, how does someone find out about changes in function signatures? Read the PRs I guess?
There was a problem hiding this comment.
yep, 'git diff' can generate a changelist for you between 2 versions
On Wed, Mar 26, 2014 at 11:37 AM, Sho Kuwamoto [email protected]:
In kew.js:
*/
-function delay(delayMs, returnVal) {
+function delay(delayMsOrVal, opt_delayMs) {I was specifically concerned about folks we may not know about who may be
relying on existing behavior (probably a very small number at this point,
but still...). If something breaks, how does someone find out about changes
in function signatures? Read the PRs I guess?Reply to this email directly or view it on GitHubhttps://github.com//pull/35/files#r10982039
.
There was a problem hiding this comment.
at some point we might want to add a changelog.
|
LGTM! |
| @@ -64,7 +64,7 @@ Promise.prototype.getContext = function () { | |||
| /** | |||
| * Resolve this promise with a specified value | |||
| * | |||
| * @param {*} data | |||
| * @param {*=} data | |||
There was a problem hiding this comment.
I just fixed some warnings in zcache because of this :)
|
LGTM |
|
updated the docs, and bumped the minor version |
Change the type signature on Q.resolve, and add Promise.prototype.delay
Hello @X-ma,
Please review the following commits I made in branch 'nick-compiler'.
e929ad6 (2014-03-26 11:00:52 -0400)
Change the type signature on Q.resolve, and add Promise.prototype.delay
R=@X-ma