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
Handle undefined in Cloud Code #682
Handle undefined in Cloud Code #682
Conversation
integration/test/ParseCloudTest.js
Outdated
@@ -70,6 +70,13 @@ describe('Parse Cloud', () => { | |||
} | |||
}); | |||
|
|||
it('run function with undefined', (done) => { | |||
Parse.Cloud.run('CloudFunctionUndefined', {}).then((result) => { | |||
assert.equal(result, null); |
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.
Why null? Should be undefined
throw new ParseError( | ||
ParseError.INVALID_JSON, | ||
'The server returned an invalid response.' | ||
); |
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 believe we should still throw an error if there was a .result before but not after decoding.
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 don’t think that would ever happen
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.
Ok then!
src/Cloud.js
Outdated
ParseError.INVALID_JSON, | ||
'The server returned an invalid response.' | ||
); | ||
return Promise.resolve(null); |
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.
To match what iOS doesf we should do the decoding on the .result and return whatever the returned value is
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.
.decode handles that internally
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.
So let’s just return the decoded result, defined or undefined. Perhaps we could check that only the result key is at least définined and no other
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 though about that as well. But let say we wanted to add additional response to cloud code like status OK or something
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.
Then we will revisit, so far we have this failing because undefined is not serialized where all other platforms support it.
request.mockReturnValue(Promise.resolve({ | ||
success: false | ||
})); | ||
request.mockReturnValue(Promise.resolve(undefined)); |
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 can probably remove the whole test then
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 change it, need for unit test
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 think the test should stay the same. Returning an object with unknown key should not be valid.
Ie: if the response has not .result and there is at least a key, then we fail
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.
Oh I see what you mean
Codecov Report
@@ Coverage Diff @@
## master #682 +/- ##
==========================================
+ Coverage 85.68% 85.69% +0.01%
==========================================
Files 48 48
Lines 3876 3880 +4
Branches 879 882 +3
==========================================
+ Hits 3321 3325 +4
Misses 555 555
Continue to review full report at Codecov.
|
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.
LGTM
Closes #417
Based on discussion parse-community/parse-server#5053
Previously threw Invalid response and unhandled promise rejection