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

Fix for callback invocation with NO_RESULT #61

Merged
merged 2 commits into from Mar 13, 2015

Conversation

Projects
None yet
4 participants
@CJRChang
Copy link
Contributor

CJRChang commented Mar 12, 2015

Due to the enumeration of cordova.callbackStatus (NO_RESULT = 0 and OK = 1), the logic used for defaulting to cordova.callbackStatus.OK will always default to cordova.callbackStatus.OK when the callbackStatus is set to cordova.callbackStatus.NO_RESULT.

This change addresses this issue by first checking if callbackOptions.status is null and defaulting to cordova.callbackStatus.OK, otherwise it will accept the callbackOptions.status value provided by the user.

Fix for callback invocation with NO_RESULT
Due to the enumeration of cordova.callbackStatus (NO_RESULT = 0 and OK = 1), the logic used for defaulting to cordova.callbackStatus.OK will always default to cordova.callbackStatus.OK when the callbackStatus is set to cordova.callbackStatus.NO_RESULT.

This change addresses this issue by first checking if callbackOptions.status is null and defaulting to cordova.callbackStatus.OK, otherwise it will accept the callbackOptions.status value provided by the user.
@@ -60,18 +60,32 @@ module.exports = function (success, fail, service, action, args) {
// CB-5806 [Windows8] Add keepCallback support to proxy
onSuccess = function (result, callbackOptions) {
callbackOptions = callbackOptions || {};
var callbackStatus;
if (callbackOptions.status !== null) {

This comment has been minimized.

@robpaveza

robpaveza Mar 12, 2015

Contributor

Is undefined a possible value here?

This comment has been minimized.

@CJRChang

CJRChang Mar 12, 2015

Author Contributor

Yes it is - in the event that the user passes a callbackOptions but does not specify "status", it will return null.

This comment has been minimized.

@robpaveza

robpaveza Mar 12, 2015

Contributor

If the user passed undefined explicitly, it would have evaluated to falsy and in line 65 (pre-change) would have been replaced with cordova.callbackStatus.OK. With your change, line 64's (post-change) test is true (undefined !== null), so local callbackStatus becomes undefined (line 65), and the status property is undefined (line 72 or 88).

This comment has been minimized.

@CJRChang

CJRChang Mar 12, 2015

Author Contributor

My sincere apologies, it seems I misunderstood your first comment - you're quite right and I've amended the code to handle undefined. Thanks for the catch!

@purplecabbage

This comment has been minimized.

Copy link
Contributor

purplecabbage commented Mar 13, 2015

Hi Ray,

I have created an issue in JIRA for this, thanks for bringing it to my attention.
I have merged your pull request into my own. here: #62
Please have a look and review it.
I removed some of the strict checking as != null will return true for both undefined and null, but not 0

Here's the long form proof ... I also considered using hasOwnProperty

// Loose equality
({status:undefined}).status != null // false
({status:undefined}).status != undefined // false
({status:undefined}).status != 0 // true

({status:null}).status != undefined // false
({status:null}).status != null // false
({status:null}).status != 0 // true

({status:0}).status != null // true
({status:0}).status != undefined // true
({status:0}).status != 0 // false

// Strict equality
({status:undefined}).status !== null // true
({status:undefined}).status !== undefined // false
({status:undefined}).status !== 0 // true

({status:null}).status !== null // false
({status:null}).status !== undefined // true
({status:null}).status !== 0 // true

({status:0}).status !== null // true
({status:0}).status !== undefined // true
({status:0}).status !== 0 // false

({status:undefined}).hasOwnProperty('status') // true
({status:null}).hasOwnProperty('status') // true
({status:0}).hasOwnProperty('status') // true
({}).hasOwnProperty('status') // false
@CJRChang

This comment has been minimized.

Copy link
Contributor Author

CJRChang commented Mar 13, 2015

Hi Jesse,
Thanks for raising the issue on JIRA - the changes look great, definitely more elegant than my initial proposal. Thanks for the input!

@@ -60,18 +60,32 @@ module.exports = function (success, fail, service, action, args) {
// CB-5806 [Windows8] Add keepCallback support to proxy
onSuccess = function (result, callbackOptions) {
callbackOptions = callbackOptions || {};
var callbackStatus;
if (callbackOptions.status !== null && callbackOptions.status !== undefined) {

This comment has been minimized.

@robpaveza

robpaveza Mar 13, 2015

Contributor

So resolution sounded like if (callbackOptions.hasOwnProperty('status')) ? That ensures that even for falsy values, if they were set, they're propagated through as-is?

@purplecabbage

This comment has been minimized.

Copy link
Contributor

purplecabbage commented Mar 13, 2015

No, that would/could work also, but I decided to just use the loosely falsey comparison
if (callbackOptions.status != null) ...
which is true for both null and undefined. See #62

@asfgit asfgit merged commit abc7715 into apache:master Mar 13, 2015

1 check passed

continuous-integration/appveyor AppVeyor build succeeded
Details

@CJRChang CJRChang deleted the CJRChang:patch-1 branch Mar 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment