Set Content-Length header in NodeHTTPTransport to disable chunked encoding

[MichaelAquilina]

Changes

Disable Chunked http encoding for unary requests in NodeHttpTransport.

Chunked encoding is only useful when we do not know what the size of our message will be. Using Raw http encoding means less overall data sent which should be more efficient.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding#Chunked_encoding

Setting the Content-Length will disable node's http library default chunked encoding:

https://nodejs.org/api/http.html#http_http_request_url_options_callback

Verification

Tested it locally with my own project.

[jonny-improbable]

Hey @MichaelAquilina, thank you for contributing 🙌

If I understand this change correctly then the content-length header should only set for unary requests. If that's correct please can you make this change? FYI, you can determine the request type from via the options member property.

It would also be great if you could look to add a unit test for this option; however I appreciate that this package has not test framework - however I would be happy to help guide you on how to contribute such a test if you have the time and interest.

[jonny-improbable]

Only set content-length for unary requests.

[johanbrandhorst]

Potentially superceded by #446.

[jonny-improbable]

@MichaelAquilina are you still planning on working on this issue?

[MichaelAquilina]

Hi @jonny-improbable sorry the delay! Working on it right now however I am struggling to understand how you would identify a unary request from the options you pointed out (looking at https://github.com/MichaelAquilina/grpc-web/blob/1860f230f669c03ef3e410391f6888548f184573/client/grpc-web/src/transports/Transport.ts#L23-L30)

[jonny-improbable]

Hey @MichaelAquilina, that's great news :)

I am struggling to understand how you would identify a unary request from the options you pointed out

The methodDefinition property should give you what you need - in particular the requestStream and responseStream members of that object - here's the relevant API Reference.

[MichaelAquilina]

From what I understand we only care about checking if the requestStream is false but you mentioned responseStream in your comment too. Perhaps I am missing something?

[jonnyreeves]

If you only want to set the content-length header for unary methods you will need to check that both requestStream and responseStream are false.

[MichaelAquilina]

Updated :)

[jonny-improbable]

Fantastic; 3 things

1. You have a linting error: src/index.ts(21,64): error TS2551: Property 'option' does not exist on type 'NodeHttp'. Did you mean 'options'? - this makes me concerned that this change may have not been tested - please can you confirm with a manual test again?
2. Please can you update the Description of this PR to reflect the fact it only disables chunked-encoding for unary requests - this is linked to from the changelog.
3. Tests would be ace, however I appreciate that no unit testing framework has been put in place for this transport - I would really appreciate it if you were in a position to copy the jest configuration from the grpc-web-fake-transport sibling package and add a unit test for this business logic; however I understand if that's not possible (and I will aim to backfill at a later date).

Thanks.

[MichaelAquilina]

1 is done.

I can attempt 2 but I suspect I might be slow to actually implement this as I have very little experience with jest.

Happy to attempt it if you are patient enough for it though :)

[jonny-improbable]

@MichaelAquilina I'm going to merge this PR in-place so as to unblock others in your position. I've cut #519 to remind us to backfill the missing test.