Add custom highWaterMark option to clone method

[nikdo]

Fixes #386.

I've spent enormous time investigating the conditions when backpressure occurs, yet I still have a lot of unanswered questions. Although I hate uncertainties, the formula I introduced is a verified hypothesis. I'm well prepared and open to any discussions about this topic. :)

I would appreciate any feedback on the code, especially tests.

[bitinn]

Many thx for this PR, I will look into as soon as I am available, it’s a much needed improvement. If anyone on the node-fetch team want to comment on this please feel free to. It’s not a part of the Fetch spec but browsers definitely have their ways of handling large streams. (From my phone)

…

On Jan 7, 2019, at 4:05, Radek Matěj ***@***.***> wrote: Fixes #386. I've spent enormous time investigating the conditions when backpressure occurs, yet I still have a lot of unanswered questions. Although I hate uncertainties, the formula I introduced is a verified hypothesis. I'm well prepared and open to any discussions about this topic. :) I would appreciate any feedback on the code, especially tests. You can view, comment on, or merge this pull request online at: #563 Commit Summary Test clone timeout Mock response instead test-specific URL Test resolved clone Create chai helper for testing promise timeout Move chai timeout helper to separate file Lower timeout value to speed up test Return mock response URL from mockResponse Add highWaterMark param to clone method Update information about clone parameter Add more extensive highWaterMark documentation Test described highWaterMark formula File Changes A CLONE-HIGHWATERMARK.md (108) M LIMITS.md (11) M src/body.js (10) M src/response.js (5) A test/chai-timeout.js (17) M test/server.js (14) M test/test.js (67) Patch Links: https://github.com/bitinn/node-fetch/pull/563.patch https://github.com/bitinn/node-fetch/pull/563.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[bitinn]

Hey @nikdo, I looked into it and while your documentation is very good. I am thinking about 2 things:

• Personally, I don't see a strong reason to expose clone(highWaterMark) option. The reason being, it's not spec-compliant, hence not isomorphic. It seems like people facing this clone problem are using node-fetch in an isomorphic sense, which is why they run into highWaterMark issue, because they expect nodejs to have the same limit as browsers.

• I would prefer 2 other solutions:

  (a) Can we pass such value via Fetch option, which means at least the browsers can ignore it and not cause a problem?

  (b) Better yet, can we simply provide a helper library that takes a node-fetch response and clone them in a way that allow for large response? And still provide the same API like text() and json()? Basically a wrapper that's isomorphic.

[gutenye]

What's the status of this PR? What can I do to help advance it? It's a pretty serious problem considering a lot of users using node-fetch in an isomorphic sense. Yet no fix in almost 3 years.

[xxczaki]

@gutenye We are currently in process of preparing the v3 release of node-fetch and this issue is in our roadmap.

I've just submitted a pull request, that fixes this problem, but in a preferred way 😄

[nikdo]

Let's close this then.