Skip to content
This repository has been archived by the owner on Nov 13, 2023. It is now read-only.

Request to implement Accept-encoding: gzip header #318

Closed
dankox opened this issue Nov 7, 2019 · 4 comments · Fixed by #526
Closed

Request to implement Accept-encoding: gzip header #318

dankox opened this issue Nov 7, 2019 · 4 comments · Fixed by #526

Comments

@dankox
Copy link

dankox commented Nov 7, 2019

AbstractRestClient.performRest() method should introduce an option (or maybe just do it by default) to include request header Accept-encoding: gzip deflate or something similar.

It should also deal with the response, when it comes in such format by checking
content-encoding: gzip or content-encoding: deflate.

This can be done by using zlib library and pipe-ing response to it, something like this:

let stream: http.IncomingMessage | zlib.Unzip = response;
// if compressed, pipe it to uncompression function
if (response.headers['content-encoding'] === 'gzip' || response.headers['content-encoding'] === 'deflate') {
	stream = zlib.createUnzip();
	response.pipe(stream);
}

stream.on('data', chunk => {
	data.push(chunk);
	size += chunk.length;
});

Currently it's not possible to do with the functionality provided by imperative, because we don't have access to response headers (or at least I don't know how) therefore not able to see if the response is compressed.
We either have to implement the whole HTTP request, or use default setup which means, the times for download from mainframe are bigger.

Just for comparison on Endevor Webservice, when using the default method for list elements, it will take over 1 minute for 8k elements, while with this implementation it will take something around 15-20 seconds.
Note: times can vary, depending on mainframe performance

@dkelosky
Copy link
Contributor

dkelosky commented Nov 7, 2019

Hi @dankox - you should be able to access the response headers via something like this:

        const client = new RestClient(session);
        await client.performRest(...);
        console.log(client.response.headers); // print or parse

@dankox
Copy link
Author

dankox commented Nov 8, 2019

Hi @dkelosky , thanks for the suggestion. I didn't know we can access it like that.

I will open issue on our repo to change some requests to this.
Just another quick question, can you access also body as a Buffer type? I remember that it was always translated to String (UTF-8).
But now seeing this way to access stuff, I see there client.data property, but I'm not sure if that will contain just the Buffer from response body, or I need to go to client.response.xxx to get it.

Anyway, regarding this issue, I guess it's more up to you if you want to implement it or not. Lot of rest clients have it as default (that's why I spot it, as it was always slower thru nodejs than any other client I was using for tests), but on the other hand the question is if imperative should update your header without your knowledge or not.
So it's up to you, you can close the issue if no update will happen in imperative.

@dkelosky
Copy link
Contributor

dkelosky commented Nov 8, 2019

Thanks @dankox. Hopefully it's clear that other public methods beyond performRest() can be run this way. client.data should be the Buffer type (assuming you're not streaming a large response). Please open an issue if this is not the case, because it's what we document.

I think an enhancement is fine idea, especially if other clients have that by default. Along the lines, I hope to have #316 pushed soon :) .

@dankox
Copy link
Author

dankox commented Nov 13, 2019

Thanks @dkelosky, we will go with your suggestions for now. And if this would be implemented, we can change it afterwards.

#316 makes sense 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants