Skip to content
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

Added support for automatic JSON response parsing. #6

Merged
merged 4 commits into from Mar 12, 2014

Conversation

@cjihrig
Copy link
Contributor

cjihrig commented Mar 12, 2014

Fixes #3

clearTimeout(clientTimeoutId);
reader.removeAllListeners();
res.removeListener('error', onError);
res.removeListener('close', onClose);
res.on('error', Hoek.ignore);

if (!err && options.parseJSON === true) {
return sendJSON();

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

Why create sendJSON() instead of just putting that logic in here?
Also, easier to read if you check (err || !options.parseJSON) and return the raw output first, then handle JSON case.

exports.read(res, function (err, payload) {
exports.read(res, options, function (err, payload) {

if (options.parseJSON !== true) {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

Better to test if the payload is not a string vs. specifically for JSON.


try {

return callback(err, JSON.parse(buffer.toString()));

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

Never call a callback inside your own try...catch.

var contentType = ((res.headers && res.headers['content-type']) || '').toLowerCase();

if (contentType !== 'application/json') {
return callback(Boom.internal('JSON not returned'), null);

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

Requesting to parse JSON doesn't mean making it an error if the response wasn't JSON.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 12, 2014

Author Contributor

I made this decision based on my own experience requesting JSON data. If you know that an endpoint should respond with JSON, but doesn't, then you have to do additional checks in your application logic to see what you got back. I figured this way you either get the expected JSON object or null.

@@ -202,12 +202,32 @@ exports.read = function (res /*, [options], callback */) {

var finish = function (err, buffer) {

var sendJSON = function () {
var contentType = ((res.headers && res.headers['content-type']) || '').toLowerCase();

This comment has been minimized.

Copy link
@hueniverse
clearTimeout(clientTimeoutId);
reader.removeAllListeners();
res.removeListener('error', onError);
res.removeListener('close', onClose);
res.on('error', Hoek.ignore);

if (!err && options.parseJSON === true) {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

I prefer calling the option just json.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 12, 2014

Author Contributor

I initially called it json but changed it to avoid confusion with request, which also sets a header. I will change it back.

… bit of restructuring.
@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 12, 2014

@hueniverse I've made all of the changes except for the error handling because I don't know your expected behavior for the two current error conditions (different MIME type and failed JSON.parse()).

return callback(err, buffer);
if (err || options.json !== true) {
return callback(err, buffer);
} else {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

No need for else.

}

try {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

No empty line

try {

json = JSON.parse(buffer.toString());
} catch (parseError) {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

catch on new line, and replace parseError with just err

json = JSON.parse(buffer.toString());
} catch (parseError) {

err = Boom.internal('Invalid JSON returned');

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

You can call callback directly from catch, just not inside try. And you should just return the error in the callback as-is.

var json = null;

if (mime !== 'application/json') {
return callback(Boom.internal('JSON not returned'), null);

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

This is not an error, just return Buffer.toString() ignoring the json flag

This comment has been minimized.

Copy link
@cjihrig

cjihrig Mar 12, 2014

Author Contributor

The normal case returns the buffer object itself. Is there a reason for returning a string in this case?

This comment has been minimized.

Copy link
@hueniverse

hueniverse Mar 12, 2014

Member

No, should be a Buffer in the non-helper case.

@cjihrig

This comment has been minimized.

Copy link
Contributor Author

cjihrig commented Mar 12, 2014

@hueniverse updated

@hueniverse hueniverse added this to the 2.4.0 milestone Mar 12, 2014
@hueniverse hueniverse self-assigned this Mar 12, 2014
hueniverse added a commit that referenced this pull request Mar 12, 2014
Added support for automatic JSON response parsing.
@hueniverse hueniverse merged commit fef4bb5 into hapijs:master Mar 12, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
hueniverse added a commit that referenced this pull request Mar 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.