Handle long streams as inputs in HTTP binding

[relu91]

This simple PR fixes #1366 by forcefully passing the request body to the node-fetch operation. I did some experiments. Basically, previously, the content parameter in the private fetch function was redundant, as it was never actually passed. That surprised me, Then I looked in the generateFetchRequest and found that we were already handling the body in that function. Removing it didn't solve the problem, but then I noticed that if I forcefully pass the body directly in the fetch RequestInit argument, then the stream is consumed correctly. I reproduced the error (the need to pass the body to request init) without using node-wot at all so I assumed that there should be something wrong with node-fetch (digging into its code, there is probably something when it clones the ReadableStream). Therefore, I left a comment for future review. Headers and other parameters seem not to be impacted.

While I was at it, I upgraded node-fetch to the last compatible version (3.x works only with ESM).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.14%. Comparing base (53a9c66) to head (cc5352c).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1369      +/-   ##
==========================================
+ Coverage   77.63%   78.14%   +0.50%     
==========================================
  Files          83       79       -4     
  Lines       16603    15252    -1351     
  Branches     1611     1442     -169     
==========================================
- Hits        12890    11918     -972     
+ Misses       3682     3317     -365     
+ Partials       31       17      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[danielpeintner]

LGTM

The "bug" is tracked by #1366 also

[danielpeintner]

Two more points:

1. The initial failing tests were a CI issue with codecov but the browser tests are still failing
2. Maybe @RRebekka can check out the fix also

[relu91]

The initial failing tests were a CI issue with codecov but the browser tests are still failing

Yes, it's a good thing that we introduced them 😆 . I've already a fix, I might publish it this afternoon cause I wanted to learn more about why it is not working... it is related to #790 .

[relu91]

I did some tests and I concluded that it is safer to keep the current approach -> translate the payload to a buffer and then send it. As far as I discovered things moved a bit from #790 , but the support is not still widely available. Moreover, in my tests, I found that Chromium (inside playwright) requires HTTP/2 (or even 3) if you want to send a body streaming in a request. If we want to support this, I'm afraid we will need to try it on many test configurations (browsers, and with or without a http/2 server).

I'd keep an eye out for further use cases, but I think that the current implementation is not that bad, even if it is less performant than a full streaming approach.

[danielpeintner]

LGTM

The only thing I don't really like too much is the overuse of fetch methods and the confusion that comes along with it ;-)
We have the actual fetch(...) and then we have HttpClient.fetch(...).

_fetch(...) is fine since it is another name

Maybe we can think about renaming the private HttpClient.fetch() but it was the case already before... so maybe it is fine to leave it as is...

[relu91]

LGTM

The only thing I don't really like too much is the overuse of fetch methods and the confusion that comes along with it ;-)
We have the actual fetch(...) and then we have HttpClient.fetch(...).

_fetch(...) is fine since it is another name

Maybe we can think about renaming the private HttpClient.fetch() but it was the case already before... so maybe it is fine to leave it as is...

Agreed, any suggestions for a possible naming refactoring?? doOperation?

[danielpeintner]

Maybe just doFetch()

Naming is always difficult... 🤷‍♂️

[danielpeintner]

LGTM