-
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
[Refactor] Use promises in 'NetInfo' #5512
Conversation
By analyzing the blame information on this pull request, we identified @bestander, @mkonicek and @andreicoman11 to be potential reviewers. |
metered => callback(metered), | ||
error => callback(null, error) | ||
); | ||
} else { |
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.
undefined This type is incompatible with Promise
@bestander Can you review this one please as you've worked on the |
looking into it... |
@@ -233,14 +230,25 @@ const NetInfo = { | |||
}, | |||
}, | |||
|
|||
isConnectionExpensive(callback: (metered: ?boolean, error?: string) => void): void { | |||
isConnectionExpensive(): Promise { | |||
if (arguments.length > 1) { |
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.
Are you sure about > 1?
arguments[0] is the first argument.
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.
@bestander My bad, should be 0. Will fix.
@satya164 updated the pull request. |
metered => callback(metered), | ||
error => callback(null, error) | ||
); | ||
} else { |
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.
undefined This type is incompatible with Promise
I'll let @bestander review. |
Thanks for doing this! |
@mkonicek I need to update the PR :) |
Please introduce the suggested deprecated wrapper in a separate PR so we can see that, then update this on top of that one. I'd like not to combine that in here. |
@satya164 Thanks, left some comments over there. @bestander you should check that out as well since this was your idea initially I think. |
@satya164 updated the pull request. |
@satya164 updated the pull request. |
@dmmiller updated |
@@ -36,13 +36,19 @@ module.exports = function(promise: Promise, callbacks: Array<Function>, type: st | |||
res => success(res), | |||
err => error(err) | |||
); | |||
case 'node': // handles func(callback) | |||
const [ callback ] = callbacks; | |||
case 'single-callback-value-first': // handles func(callback(err, value)) |
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.
The comment indicates error first but the case indicates value first.
@satya164 updated the pull request. |
@facebook-github-bot-1 shipit |
@facebook-github-bot shipit |
1 similar comment
@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/426911070839764/int_phab to review. |
This is merged. |
No description provided.