clone() hangs with large response in Node about ky-universal HOT 8 CLOSED

@gutenye @sindresorhus In version 3.0.0 of node-fetch you will be able to manually set highWaterMark and prevent this issue from happening 😄

fetch(url, {highWaterMark: 10}).then(res => res.clone().buffer());

See this PR

from ky-universal.

It would be pretty challenging to re-implement .clone() ourselves. In doing so, we would probably end up creating more new bugs than we solved. The only practical way forward that I see here would be to just not clone the response at all.

We currently clone in three places:

1. When calling afterResponse hooks
2. When streaming the response to provide progress events
3. When body method shortcuts are used (e.g. ky().json())

I'm not sure which, if any, of these places would be safe to remove. None of them seem particularly important to me, though I like having them for safety.

It's worth noting that we only call .clone() if you use one of the above mentioned features. So you can actually avoid this problem if you can live without those features.

For example, if .clone() is being called because you are using the .json() method like this:

const users = await ky.get('users').json();

It's perfectly fine to re-write that to:

const response = await ky.get('users');
const users = await response.json();

... in which case .clone() is not called. The only caveat, beyond it being more verbose, is that you will also have to set the Accept: application/json header yourself, if you need that.

Other than those workarounds, I'm not sure there's much we can do, since we are using the API correctly and this is really an upstream issue. We could explore alternative fetch polyfills, but I suspect that would again introduce its own set of problems. Let's keep an eye on that issue you referenced.

In the meantime, should we consider removing .clone() internally and, if so, where? Removing 3 seems the safest to me and it's probably the most common case as well.

from ky-universal.

I don't think we should remove .clone(). We should rather get node-fetch fixed.

I'm gonna add a bounty here that applies to getting node-fetch/node-fetch#386. If you want to work on it, see: node-fetch/node-fetch#563 (comment)

from ky-universal.

@exogen on 15th of May 2019:

Here's a simple repro: https://github.com/exogen/ky-afterResponse-repro

This doesn't seem to happen for every request; I suspect it might have something to do with the response body size. I tried a couple small public JSON API demos before I landed on a GraphQL endpoint that reproduced the behavior.

Under certain conditions, simply including an afterResponse hook causes the response Promise to hang. The hook doesn't have to do anything; it can be a no-op function or just return the same cloned response. Removing the hook causes it to start working. Switching to node-fetch also works fine.

I wonder if the hook causes the response to be cloned and therefore the Promise as well, making one of them never resolve? Just a thought.

from ky-universal.

@issuehunt has funded $80.00 to this issue.

• Submit pull request via IssueHunt to receive this reward.
• Want to contribute? Chip in to this issue via IssueHunt.
• Checkout the IssueHunt Issue Explorer to see more funded issues.
• Need help from developers? Add your repository on IssueHunt to raise funds.

from ky-universal.

Drive by comment as it seems like wheels are already in motion to fix this: I think this should be added to the README.

I've hit this behaviour while writing up endpoint methods for an api in our electron app; b/c I was being lazy I was just running the code via ts-node to test the responses & such, rather than recompling the whole app and running electron.

I was really confused for a while, getting ready to deep dive into ky thinking I might have found a major bug, since it looked like just nothing - node would just exit w/o any errors, and no promises would resolve.

Given that it seems switching from .json() to .then(r => r.json()) will fix this for node w/ no downside in either browser or node, I think it's worth sticking that in the README 😄

from ky-universal.

Thanks @sholladay. I can confirm the workaround works for my case with .json().

from ky-universal.

@sindresorhus has rewarded $72.00 to @xxczaki. See it on IssueHunt

• 💰 Total deposit: $80.00
• 🎉 Repository reward(0%): $0.00
• 🔧 Service fee(10%): $8.00

from ky-universal.