ky-universal fetch leads to body timeout errors

[paambaati]

Bug report

I use ky-universal with Next.js to handle all my API calls, and I use ky's hooks (middleware for APIs) to do things like logging, handling cookies, etc.

The library had a long-standing bug with timeout errors on large response bodies, and it was recently fixed when they switched their node-fetch dependency to 3.0.0-beta.6. Note that these are not actually timeouts from the API itself, but timeouts while trying to consume the response stream and parsing it as JSON. The linked PR was supposed to fix these issues (similar to mine) —

1. clone() hangs with large response in Node sindresorhus/ky-universal#8
2. afterResponse hook causes response Promise to hang sindresorhus/ky#135
3. afterResponse hook stops response from resolving sindresorhus/ky-universal#18
4. .json() maximum size/length? sindresorhus/ky-universal#15

After this change, I'd expected my hooks to work again, but I'm still getting the same errors —

FetchError: Response timeout while trying to fetch https://my-api.com/my-api (over 30000ms)
    at Timeout._onTimeout (/my-project/node_modules/next/dist/compiled/node-fetch/index.js:1:134853)
    at listOnTimeout (internal/timers.js:549:17)
    at processTimers (internal/timers.js:492:7) {
  type: 'body-timeout'
}

This leads me to believe Next.js's own version of node-fetch is shadowing the one that ky-universal should use.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

1. See https://github.com/paambaati/ky-body-timeout-repro
2. Use https://github.com/paambaati/ky-body-timeout-repro/blob/234f5b9ca9af5de576b1b86ead650b29daaef66c/index.js#L28 and the associated api() method in a Next.js API route with ky-universal dependency set to version 0.7.0 - this version includes the fix for timeouts.
3. Hit the Next.js API route and watch it timeout for responses roughly over 50 KB in size.

Expected behavior

1. Should not error if we're using the latest version of ky-universal.
2. Should prefer ky-universal's node-fetch over Next.js's own.

System information

• OS: macOS Mojave 10.14.6
• Browser Chrome & Firefox
• Version of Next.js: 9.4.4
• Version of Node.js: 14.1.0

Additional context

Somewhat related — #12761

[Timer]

This issue stems from ky-universal using global.fetch if available, instead of the version they ship with. An issue should really be opened upstream with them about this.

We're happy to entertain how this could be fixed in Next.js though, feel free to send a PR!

[sagar-gavhane]

@paambaati Maybe you need to upgrade ky-universal to v0.7.0. I saw that this PR is released in v0.7.0.

Checkout release v0.7.0 note: https://github.com/sindresorhus/ky-universal/releases/tag/v0.7.0

[paambaati]

@sagar-gavhane This issue happens with 0.7.0 as well.

[sholladay]

Hi all, Ky maintainer here! Happy to help with this.

Just to make sure we're on the same page, Next has not yet released a version that uses ky-unversal v0.7.0 and it hasn't updated its own node-fetch dependecy to avoid the clone() bug either. What are you doing to ensure the correct versions are being used?

Also, could someone enlighten me as to why Next uses node-fetch and ky-universal? The main purpose of ky-universal is that it ships polyfills for web standard APIs that some environments are missing (mainly Node). If you're shipping your own polyfills, for whatever reason, then you should be able to use ky by itself without ky-universal.

I'm assuming that Next attaches its node-fetch dependency to the global scope, as that should be the only thing that would cause the updated node-fetch version within ky-universal to be ignored. If that's the case, then it's behaving as intended because we don't want ky-universal to overwrite any existing fetch implementation. It only uses its own if it has to. We could change ky-universal so that it has a stronger bias towards its own fetch implementation, but then we'd have the opposite problem, if ky-universal had outdated dependencies, then it would be a pain. Given that fetch ought to be part of the environment in the first place, I personally think the "use our own as plan B" approach is the right way.

[paambaati]

Also, could someone enlighten me as to why Next uses node-fetch and ky-universal?

@sholladay Just to add some more context, Next.js itself doesn’t use ky-universal. I’m building a Next.js app that happens to use ky-universal as a dependency.

[sholladay]

I see ky and ky-universal in the package.json here in the Next.js repository. Even if you're not intending to use that install, maybe it's affecting things somehow?

[paambaati]

@sholladay AFAIK, it is used only for tests — it was actually introduced in #12804

I think the problem is, Next.js also has its own polyfilled version of node-fetch as is evident from the stacktrace, and ky-universal seems to fallback to it instead of using its own.

Do you think letting users bring their node-fetch for use in ky-universal is something you’d entertain?

[sholladay]

Do you think letting users bring their node-fetch for use in ky-universal is something you’d entertain?

We've talked about it a few times and I think it's still on the table. The thing is, it needs a better use case than just a workaround for an upstream bug that's already been solved.

In a sense, bring your own fetch is already supported since we check for its existence before using our own, albeit via globals. But it's done that way because fetch is supposed to be global. Sure we could introduce an option to override both the global fetch and our internal one, but that seems pretty hacky, don't you think? Should we have such an option for each global we rely on, or only fetch because node-fetch "had a bug that one time"? I'm skeptical that this option will see much actual use beyond that, and thus I'm slightly against adding it.

All that said, given that node-fetch has caused a fair amount trouble for our users, if there's no better way to solve this then yeah, we would likely take a high quality PR for a fetch option.

[TasukuUno]

@paambaati @sholladay Hi, I'm sorry to mention suddenly. This problem also happened to me.
Are there any updates or workarounds?

[paambaati]

@TasukuUno Honestly, this might not be what you wanted to hear, but I gave up on Ky and just moved to Axios.

[TasukuUno]

@paambaati oh... It's sad to hear that... but now I understand the current situation!
Thank you for your quick response!!

[sholladay]

As of Ky v0.23.0 you can now provide a fetch option to override the fetch implementation regardless of the globals.

import ky from 'ky';
import fetch from 'isomorphic-unfetch';

const json = await ky('https://example.com', { fetch }).json();

And this is also supported with custom instances, so you don't need to specify it for every call.

Is there anything else we can do to help?

[bkiac]

As of Ky v0.23.0 you can now provide a fetch option to override the fetch implementation regardless of the globals.

import ky from 'ky';
import fetch from 'isomorphic-unfetch';

const json = await ky('https://example.com', { fetch }).json();

And this is also supported with custom instances, so you don't need to specify it for every call.

Is there anything else we can do to help?

This workaround doesn't work for me, my requests still hang with large responses

[balazsorban44]

Using Next.js 12 and the following API route:

import ky from "ky-universal"

const api = () => {
  const defaultOptions = {
    prefixUrl: "https://jsonplaceholder.typicode.com",
    credentials: "include",
    keepAlive: true,
    retry: 0,
    hooks: {
      afterResponse: [
        (req, opt, res) => {
          console.log("KY -> AFTER RESPONSE HOOK!")
          // The below line does not affect the outcome when it is uncommented either.
          // return res;
        },
      ],
    },
  }

  return ky.create(defaultOptions)
}

export default async function handler(_, res) {
  res.json(await api().get("todos/1").json())
}

I'm no longer able to reproduce any issue here. I suggest upgrading Next.js, and if you still experience the problem, please open a new bug report.

[github-actions]

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.