-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Add a deprecatedCallback helper #5861
Conversation
👍 Very cool and welcome back from vacation! :) Will let @dmmiller review. |
|
||
'use strict'; | ||
|
||
// @deprecated, will be removed on March 1 2016 |
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.
Does this mean we'll break everyone still using callbacks? We could give people a bit more time.
@mkonicek Originally I was supposed to work on it 2 weeks back, so the date, changed it to April 1 :D |
@satya164 updated the pull request. |
|
||
'use strict'; | ||
|
||
// @deprecated, will be removed on April 1 2016 |
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.
We probably won't remove it then because we may have others that we will get to over time. Maybe we should pass the timeline in to the deprecatedCallback explicitly and add it to the warning that is output.
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.
Won't we have almost 3 releases before April 1st?
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.
Yea, I'd just rather not commit us to a particular time frame on the utility function. We may need something like this later for other things.
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.
@dmmiller Removing.
In general, definitely in the right direction. Thanks for doing this! |
@satya164 updated the pull request. |
res => success(res), | ||
err => error(err) | ||
); | ||
case 'node': // handles func(callback) |
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.
Is this right name?
Node.js convention is callback(error, success...)
.
In this case this is single-callback-success-first
I suppose.
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.
Sorry typo. Functions are wrong. Fixing.
Great work! |
@satya164 updated the pull request. |
@satya164 updated the pull request. |
looks good to me, @dmmiller ? |
@satya164 updated the pull request. |
The update branch button is very nice :D |
@satya164 updated the pull request. |
The bot hates me :( |
bot hates all of us. |
Looking into fixing the bot now. |
@facebook-github-bot shipit |
@satya164 updated the pull request. |
@facebook-github-bot shipit |
@satya164 updated the pull request. |
@satya164 updated the pull request. |
@facebook-github-bot shipit |
Let's see if the bot is working again :) |
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/976782889081674/int_phab to review. |
Sorry for so long, was very busy last 2 weeks and was on vacation during the weekend :)
cc @dmmiller @bestander