refactor(rest): switch from undici request to global fetch

[ckohen]

BREAKING CHANGE: The raw method of REST now returns a web compatible Response object. Additionally, many underlying internals have changed, some of which were exported.

Please describe the changes this PR makes and why it should be merged:

Undici request was originally chosen for performance reasons. At the scale of discord bots, this performance difference is irrelevant. This instead is the first step towards cross environment compatability.

Status and versioning classification:

• Code changes have been tested against the Discord API, or there are no code changes
• I know how to update typings and have done so, or typings don't need updating
• This PR includes breaking changes (methods removed or renamed, parameters moved or removed)

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
discord-js	⬜️ Ignored (Inspect)			May 6, 2023 1:06pm
discord-js-guide	⬜️ Ignored (Inspect)			May 6, 2023 1:06pm

[codecov[bot]]

Codecov Report

Merging #9416 (c0dccd6) into main (28f5179) will increase coverage by 0.15%. The diff coverage is 95.73%.

:exclamation: Current head c0dccd6 differs from pull request most recent head 5c7cf19. Consider uploading reports for the commit 5c7cf19 to get more accurate results

@@            Coverage Diff             @@
##             main    #9416      +/-   ##
==========================================
+ Coverage   58.55%   58.70%   +0.15%     
==========================================
  Files         226      227       +1     
  Lines       14765    14756       -9     
  Branches     1256     1268      +12     
==========================================
+ Hits         8645     8663      +18     
+ Misses       6080     6053      -27     
  Partials       40       40              

Flag	Coverage Δ
proxy	75.65% <90.00%> (-0.35%)	:arrow_down:
rest	93.12% <96.10%> (+1.22%)	:arrow_up:
ws	54.38% <ø> (+0.07%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/proxy/src/util/responseHelpers.ts	80.95% <90.00%> (-0.76%)	:arrow_down:
packages/rest/src/lib/handlers/Shared.ts	88.38% <90.90%> (-0.15%)	:arrow_down:
...ackages/rest/src/lib/handlers/SequentialHandler.ts	87.74% <93.75%> (ø)
packages/rest/src/strategies/undiciRequest.ts	94.28% <94.28%> (ø)
packages/rest/src/lib/REST.ts	98.91% <100.00%> (+0.03%)	:arrow_up:
packages/rest/src/lib/RequestManager.ts	90.07% <100.00%> (-0.10%)	:arrow_down:
packages/rest/src/lib/handlers/BurstHandler.ts	91.09% <100.00%> (ø)
packages/rest/src/lib/utils/constants.ts	96.96% <100.00%> (+0.09%)	:arrow_up:
packages/rest/src/lib/utils/utils.ts	94.73% <100.00%> (-1.72%)	:arrow_down:

... and 11 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[KhafraDev]

This is a horrible, horrible change. I chose undici.request because fetch is not ready for production use, but mostly so people's bots aren't 6x+++ slower in every single http request.

1. It's slow. Web streams are also slow(er than node streams). -- https://github.com/nodejs/undici/issues/1203 -- https://github.com/nodejs/performance/issues/11 -- https://github.com/nodejs/undici/issues/1203#issuecomment-1100969210
2. It's marked as experimental in node, because of performance. Even if undici is used the same sentiment still applies. Particularly the part where it says stage 1 experimental features are "... not recommended in production environments."

At the scale of discord bots, this performance difference is irrelevant

This is honestly just absurd, unsubstantiated, and completely wrong, lol. Maybe having 6x less requests "doesn't matter" (spoiler alert: it does, this is exactly the type of change that forces large bot creators to use eris) but you also don't account for increased memory, higher CPU usage, more GC cycles (which are slow), more TCP retransmissions (bug in fetch; packets are being dropped), and finally, general performance concerns in web streams...

This instead is the first step towards cross environment compatability.

At the expense of a majority of discord.js' users... Why is supporting both Deno users and the few bun users more important than the node.js ones?

---

If you really want to make it compatible with other environments, add in a hook system where users can use their own http client. That way platforms that offer faster alternatives (like node & deno) don't have to sacrifice performance for compatibility. Having just the url and request options, and returning a Response-like object would be enough to plug in undici.request, and thus cancel out my concerns.

import { Client } from 'discord.js'
import { request, type RequestInit, Headers } from 'undici'

interface ResponseLike {
  json (): Promise<unknown>
  headers: Headers, // Headers are fast in undici
  status: number
}

async function makeRequest (url: string, init: RequestInit): Promise<ResponseLike> {
  const { statusCode, body, headers } = await request(url, transformOpts(init))

  return {
    status: statusCode,
    headers: new Headers(headers),
    async json () {
      return await body.json()
    }
  }
}

// The default makeRequest would be
// async function makeRequest (url: string, init: RequestInit): Promise<ResponseLike> {
//    return await fetch(url, init) satisfies Promise<ResponseLike> // idk if satisfies is needed
// }

const client = new Client({
  rest: {
    makeRequest
  }
})

---

edit: I will work on this feature just so my bot doesn't suffer, as an alternative for this pr.

[ckohen]

Why is supporting both Deno users and the few bun users more important than the node.js ones?

That's not the type of cross-env we're talking about. It is far more important to have a rest implementation that works in edge at this point than supporting deno / bun (though deno is looking ever more appealing).

This is honestly just absurd, unsubstantiated, and completely wrong, lol. Maybe having 6x less requests "doesn't matter"

The majority of discord.js users have a hard 50/second cap, from discord, nowhere near the throughput of either request or fetch. Even very large bots usually only get bumped to 500/second. (Remember, there's a good portion of users still on v13, which uses node-fetch).

add in a hook system where users can use their own http client

I do like this, we'd probably export (subpath export so undici isn't imported by default), though you make it sound so much easier than it is. Notably that entire resolveBody util which you handily gloss over in your example with the unexplained transformOpts(). Additionally, ResponseLike needs to be a lot closer to Response than you made it, and quite importantly, body will have to be typed as unknown, and clone() couldn't exist.

This is a horrible, horrible change.

I appreciate the information on why you chose request (which I did know), and clearly you have a good idea of alternative ways to make this work. I especially appreciate the explanation as to why fetch is still stage 1 since it wasn't anywhere we could find and we figured it wasn't actually unstable since last we heard it wasn't going to land in core until it was stable.

I am happy to take suggestions (part of why the blocked label was added to this PR), but I would appreciate if you were a little less harsh. I understand this isn't what you want to see, but there are better ways to approach that (much of which you do, just after you express your anger)

Also, in case you were wondering, this won't make it to djs v14.

[KhafraDev]

When I said that it's is a horrible change I meant switching to fetch, which is what I've implemented a majority of. It was meant as a critique of my own work, rather than this PR. Fetch just isn't a good http client in node; it's slower than node-fetch (which uses node:http/s), it's slower than undici.request. It's a browser API in a server environment. On the edge it's more fitting & I would love to be able to use it in CFW, but not at the expense of my bot running node.js.

I have a draft PR implement the hook but I can't test it out because of <issues>. We can continue in Discord if you want?

[ckohen]

The latest commit will need to be squashed before this merges, leaving it separate for ease of review for now.