Skip to content

Support for binary and JSON response data (responseType other than 'text') #9

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# http://editorconfig.org
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as I like having standardized editor configurations, I'd prefer if this file were not included.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any specific reason?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, the eslint configuration already enforces a lot of these upon linting, and having fewer files is nice.

On the other hand, it's not really a big deal, so feel free to leave it in - I believe the package.json is already set up to only include stuff in lib/ upon publish.

Copy link
Author

@tchakabam tchakabam Oct 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, eslint is an automated tool to check/fix/ensure these rules.

editorconfig is configuration for your editor, making it easier to observe the rules: https://editorconfig.org/

I agree that too much tooling and config files isn't desirable, but then again, it's just a bunch of template files that you can just copy from one project to another :) but maybe we just need a more integrated toolchain in the first place in javascript hehe?

root = true
[*]
indent_style = space
indent_size = 2
end_of_line = lf
charset = utf-8
trim_trailing_whitespace = true
insert_final_newline = true
61 changes: 56 additions & 5 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ var supportedResponseTypes = Object.freeze({
/** Text response (implicit) */
'': true,
/** Text response */
'text': true
'text': true,
/** Binary-data response */
'arraybuffer': true,
/** JSON response */
'json': true
});

/**
Expand Down Expand Up @@ -139,6 +143,14 @@ module.exports = function () {
* @type {?String}
*/
this._responseText = null;

/**
* @see https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/response
*
* @private
* @type {ArrayBuffer | String | Object}
*/
this._response = null;
};

/** @alias module:node-http-xhr */
Expand Down Expand Up @@ -297,7 +309,17 @@ NodeHttpXHR.prototype = Object.create(
throw new Error('Unsupported responseType "' + type + '"');
}

return this._responseText;
switch (type) {
case '':
case 'text':
return this._responseText;
case 'json':
return JSON.parse(this._responseText);
case 'arraybuffer':
return this._response.buffer;
default:
throw new Error('Assertion failed: unsupported response-type: ' + type);
}
}
},
/**
Expand All @@ -308,10 +330,19 @@ NodeHttpXHR.prototype = Object.create(
* This will be incomplete until the response actually finishes.
*
* @type {?String}
* @throws when `response` not a String
* @readonly
*/
responseText: {
get: function getResponseText() { return this._responseText; }
get: function getResponseText() {

// @see https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/responseText#Exceptions
if (this._responseType !== 'text' && this._responseType !== '') {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could write a private method to check this, since it is done below as well

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea 👍

throw new Error('InvalidStateError: Response-type is ' + this._responseType);
}

return this._responseText;
}
},
/**
* Indicates whether or not cross-site `Access-Control` requests should be
Expand Down Expand Up @@ -469,13 +500,33 @@ NodeHttpXHR.prototype.send = function (data) {
this._resp = resp;
this._responseText = '';

resp.setEncoding('utf8');
// var contentType = resp.headers['content-type'];
// TODO: adjust responseType from content-type header if applicable

// from Node API docs: The encoding argument is optional and only applies when chunk is a string. Defaults to 'utf8'.
if (this._responseType === 'text' || this._responseType === '') {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use private method for checl

resp.setEncoding('utf8');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may need to set this for json case as well to actually get a string in on-data.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or decode the buffer ourself but that seems more elegant

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - the encoding should definitely by UTF-8 for JSON.

}

resp.on('data', function onData(chunk) {
this._responseText += chunk;

if (typeof chunk === 'string') {
this._responseText += chunk;
} else if (typeof chunk === 'object') {
if (!(chunk instanceof Buffer)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not even sure we need this check, was more of a dev-time tool

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it's good to have the assertion there, for sanity's sake. Maybe we want to have a catch-all, since there are only 2 possibilities (if I'm reading it correctly)?

} else if (chunk instanceof Buffer) {
  // as before
} else {
  throw new Error('Assertion failed: Response-data should be either a string or Buffer');
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good one. i am actually always recommending run-time assertions that slam into devs face when they break stuff, or even at runtime, instead of hiding away unexpected behavior :)

throw new Error('Assertion failed: Response-data should be of `Buffer` type');
}
if (this._response) {
this._response = Buffer.concat([this._response, chunk]);
} else {
this._response = chunk;
}
}

if (this.readyState !== this.LOADING) {
this._setReadyState(this.LOADING);
}

}.bind(this));

resp.on('end', function onEnd() {
Expand Down
25 changes: 25 additions & 0 deletions test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,31 @@ function describeXHRProps() {
});

describe('#responseType', function () {

it('supports binary data (arraybuffer responseType)', function (done) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding a test!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's say that at first i was so lost in here, that i wouldn't have been able to implement without it :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a test for json could be added as well


var responseData = new Buffer(909);

var binaryScope = nock('http://example.com')
.get('/music.mp3')
.reply(200, responseData, {
'Content-Type': 'audio/mpeg'
});

req.responseType = 'arraybuffer';
req.open('GET', 'http://example.com/music.mp3');
req.send();

req.onload = function () {

assume(req.responseType).equals('arraybuffer');
assume(req.response).equals(responseData.buffer);

binaryScope.done();
done();
};
});

it('intially is `\'\'`', function () {
assume(req.responseType).equals('');
});
Expand Down