Drain data from response in browserify environment

[krishnasrinivas]

fixes #654

[harshavardhana]

wouldn't this work for nodejs as well?

[krishnasrinivas]

it will work, but socket.end() would be the right way to close the socket. response.on('data', ()=>{}) is a workaround on browser. So it is better to do it this way, I think. (Though I see the advantage of having response.on('data', ()=>{}) for both node and browser is that you don't need if/else statements.)

[harshavardhana]

yes that is what i was thinking. Accessing socket feels too low level IMO. You let nodejs runtime to do that for you but our approach is just to drain the response body.

[vadmeste]

yes that is what i was thinking. Accessing socket feels too low level IMO. You let nodejs runtime to do that for you but our approach is just to drain the response body.

do you mean nodejs runtime will automatically close underlying socket when it frees response ?

[harshavardhana]

Well that is what I would expect. That would allow underlying http to reuse the socket.

[kaankabalak]

Wouldn't the response.socket.end() on line 1381 also return the same error?

Update: When I tried running the statObject function that encloses the response.socket.end() method mentioned above in the browser (Firefox 57.0.1), it indeed returns a similar error with the message TypeError: response.socket is undefined.

[vadmeste]

@krishnasrinivas, is there a sample project which uses minio-js in a browser ? I am not able to find one.

[harshavardhana]

@krishnasrinivas, is there a sample project which uses minio-js in a browser ? I am not able to find one.

@vadmeste https://gist.github.com/harshavardhana/9f8f2236dee975fb70edbec5db14fd67 here is a sample work.

[krishnasrinivas]

Wouldn't the response.socket.end() on line 1381 also return the same error?

good catch @kaankabalak, I will fix it.

[kaankabalak]

Test that failed previously runs fine now, LGTM

[vadmeste]

Do we support officially support browser ? Because there are no documentation about how to generate minio-browser.js (gulp browserify). I can do that in a separate PR.

[harshavardhana]

Browser is actually officially supported, lack of docs is perhaps an oversight.

[deekoder]

I don't think its an oversight. I don't think we are officially supporting it. But users are using it on the client side anyway.

[harshavardhana]

We are actually officially supporting it, I don't remember when we decided not to @deekoder . Browser side support was added to comply with aws-sdk-js behavior long time ago.

We just have to document it how to use it though.

[deekoder]

I think we not only need to discuss it further but also test it and then document it. @abperiasamy

[vadmeste]

LGTM & tested

[vadmeste]

LGTM

[harshavardhana]

It was decided that we will properly document, test this and support browser usage for minio-js.

[krishnasrinivas]

@harshavardhana we can merge this right? So that @gusajz and other browserified minio-js users are not blocked from our testing efforts. Anyway till we we document that it is supported, it will not be supported.

[harshavardhana]

@harshavardhana we can merge this right? So that @gusajz and other browserified minio-js users are not blocked from our testing efforts. Anyway till we we document that it is supported, it will not be supported.

It was decided that we merge this and move forward with documenting/testing subsequently.