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

tchakabam · 2018-10-10T00:38:11Z

Please let me know if this needs any explanation!

Cheers and thanks for the great prior work 👍

…fer/json responseType)

tchakabam · 2018-10-10T00:58:46Z

lib/index.js

+          } else {
+            this._response = chunk;
+          }
+        // binary data usually comes in one chunk (no chunky transfer-encoding)


note to self: this comment is confusing. transfer-encoding is unrelated to the chunks of data delivered by this callback (which is reading from net socket)

…y 'data' callback equally

aspyrx

Thank you very much for the contribution! Most of it looks great. See comments for further details.

Re: the tests failing, I may deprecate support for [email protected] and/or [email protected], since nock<=8.x doesn't support those anymore. As for the latest node/npm, I will look into those, since it's been ages since I last updated this package.

aspyrx · 2018-10-10T04:54:39Z

.editorconfig

@@ -0,0 +1,9 @@
+# http://editorconfig.org


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

any specific reason?

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.

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?

lib/index.js

aspyrx · 2018-10-10T05:00:50Z

test/index.test.js

@@ -413,6 +413,31 @@ function describeXHRProps() {
    });

    describe('#responseType', function () {
+
+      it('supports binary data (arraybuffer responseType)', function (done) {


Thanks for adding a test!

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

a test for json could be added as well

tchakabam · 2018-10-10T14:30:06Z

@aspyrx The tests were running perfectly fine on my machine. Except one which was stuck pending (probably missing call to async done()). The CI failures are probably due to CI env mismatch indeed, but no idea of the details. My local node version is v8.10.0.

tchakabam · 2018-10-10T14:38:45Z

lib/index.js

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


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

Seems like a good idea 👍

tchakabam · 2018-10-10T14:39:00Z

lib/index.js

+    // 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 === '') {


use private method for checl

tchakabam · 2018-10-10T14:41:14Z

lib/index.js

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


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

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'); }

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 :)

tchakabam · 2018-10-10T14:43:09Z

lib/index.js

+
+    // 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 === '') {
+      resp.setEncoding('utf8');


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

or decode the buffer ourself but that seems more elegant

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

tchakabam · 2018-10-15T11:46:48Z

@aspyrx We also need to be able to follow redirects. One possibility without having to change too much here would be using this drop-in replacement for http/https modules: https://www.npmjs.com/package/follow-redirects

But in a general way it seems that re-implementing HTTP-client-functionnality properly here with Node at the high-level API that XHR requires it, using a solution like provided by https://github.com/request/request would be better? The built-in Node stuff comes pretty much without batteries it seems, and many problems one would typically need to solve are taken care off here ^

It requires a few subtle changes but can be more or less used the same way. However that module lacks abort method.

tchakabam · 2018-10-15T15:58:11Z

see tchakabam@4785a4b

aspyrx · 2018-10-16T20:25:02Z

@aspyrx We also need to be able to follow redirects. One possibility without having to change too much here would be using this drop-in replacement for http/https modules: https://www.npmjs.com/package/follow-redirects

But in a general way it seems that re-implementing HTTP-client-functionnality properly here with Node at the high-level API that XHR requires it, using a solution like provided by https://github.com/request/request would be better? The built-in Node stuff comes pretty much without batteries it seems, and many problems one would typically need to solve are taken care off here ^

It requires a few subtle changes but can be more or less used the same way. However that module lacks abort method.

I agree that redirects need to be handled.

I'd prefer to use a drop-in replacement if possible - follow-redirects seems like a good solution. However, this discussion should probably happen in a new PR/issue.

tchakabam · 2018-10-16T21:15:09Z

However, this discussion should probably happen in a new PR/issue.

Totally agree.

tchakabam · 2018-10-16T21:19:01Z

I am also more critical the more I look at https://github.com/request/request . It takes care of many things, but has a very large baggage of dependencies it seems + the API isn't clear in all parts and it seems there is a bug related to aborting requests - which I consider being a critical issue and which would be a regression with the existing functionnality here.

The reason I looked at it was that I had a problem facing that was maybe related to redirects, but also encoding of data, but it turns out it may not have been caused by this lib, nor actually solved by lib request.

If we can make redirects work, and I can test and validate all my cases here, we should rather put efforts in building it properly but lean&mean right on top of nodes http/s.

tchakabam added 4 commits October 10, 2018 02:36

add support for binary and serialized object-data responses (arraybuf…

4eb5409

…fer/json responseType)

add test case for arraybuffer responseType

db00a75

add editorconfig

dc87e4a

Buffer-type chunks: support concatenation

3fd4eee

tchakabam commented Oct 10, 2018

View reviewed changes

tchakabam added 2 commits October 10, 2018 03:05

handle json case and also handle both ArrayBuffer and Buffer passed b…

4804d0b

…y 'data' callback equally

simplify handling of arraybuffer case and properly handle json data

0c370c9

aspyrx requested changes Oct 10, 2018

View reviewed changes

tchakabam commented Oct 10, 2018

View reviewed changes

tchakabam force-pushed the master branch from 4785a4b to 0c370c9 Compare October 15, 2018 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tchakabam commented Oct 10, 2018 •

edited

Loading

tchakabam Oct 10, 2018

aspyrx left a comment

aspyrx Oct 10, 2018

tchakabam Oct 10, 2018

aspyrx Oct 10, 2018

tchakabam Oct 12, 2018 •

edited

Loading

aspyrx Oct 10, 2018

tchakabam Oct 10, 2018

tchakabam Oct 10, 2018

tchakabam commented Oct 10, 2018

tchakabam Oct 10, 2018

aspyrx Oct 10, 2018

tchakabam Oct 10, 2018

tchakabam Oct 10, 2018

aspyrx Oct 10, 2018

tchakabam Oct 12, 2018

tchakabam Oct 10, 2018

tchakabam Oct 10, 2018

aspyrx Oct 10, 2018

tchakabam commented Oct 15, 2018 •

edited

Loading

tchakabam commented Oct 15, 2018

aspyrx commented Oct 16, 2018

tchakabam commented Oct 16, 2018

tchakabam commented Oct 16, 2018

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

Are you sure you want to change the base?

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

Conversation

tchakabam commented Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

aspyrx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchakabam Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchakabam commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchakabam commented Oct 15, 2018 • edited Loading

tchakabam commented Oct 15, 2018

aspyrx commented Oct 16, 2018

tchakabam commented Oct 16, 2018

tchakabam commented Oct 16, 2018

tchakabam commented Oct 10, 2018 •

edited

Loading

tchakabam Oct 12, 2018 •

edited

Loading

tchakabam commented Oct 15, 2018 •

edited

Loading