refactor: node-fetch & @discordjs/formdata -> undici

[KhafraDev]

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

Removed the dependencies of node-fetch and @discordjs/formdata in favor of undici.

Undici contains spec-compliant fetch and FormData implementations, is maintained by
NodeJS contributors, and is just basically all around better. Repo

Complete list of changes:

• Removed @discordjs/form-data package and its 4 dependencies.
• Removed node-fetch (still installed as a dev package)
• MessageAttachment and MessageAttachment.setFile now accept a Blob parameter.
• Likewise, DataResolver.resolveFile also accepts a Blob. It also returns a Blob now, whereas it used to return a Buffer or ReadableStream.
• DataResolver.resolveBase64 now returns Promise<string>.
• DataResolver.resolveFileAsBuffer is no longer used anywhere in the code, so like previously unused utility functions, will likely get removed in v14 (if this pr lands).
• The library now requires Node >=16.7.0 (bumped from Node >=16.6.0)

edit: I've been personally using this on my bot (roughly 21,000 members) that has made thousands of http requests and there has been no issue. This PR is likely ready for production use if anyone reading this wants to try it out. ;)

Status and versioning classification:

• 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)
• Code changes have been tested against the Discord API, or there are no code changes

[iCrawl]

https://github.com/nodejs/undici#undicifetchinput-init-promise

Only supported on Node 16+.

This is experimental and is not yet fully compliant with the Fetch Standard. We plan to ship breaking changes to this feature until it is out of experimental.

https://nodejs.org/api/documentation.html#documentation_stability_index

Stability: 1 - Experimental. The feature is not subject to Semantic Versioning rules. Non-backward compatible changes or removal may occur in any future release. Use of the feature is not recommended in production environments.

[KhafraDev]

https://github.com/nodejs/undici#undicifetchinput-init-promise

Only supported on Node 16+.

This is experimental and is not yet fully compliant with the Fetch Standard. We plan to ship breaking changes to this feature until it is out of experimental.

https://nodejs.org/api/documentation.html#documentation_stability_index

Stability: 1 - Experimental. The feature is not subject to Semantic Versioning rules. Non-backward compatible changes or removal may occur in any future release. Use of the feature is not recommended in production environments.

I completely agree with this not being ready yet, just posting it here for some discussion. I assume most of the animosity in the previous discussion linked above was that someone had to do the work, so I decided to just see what happens.

Although I believe v16 is about to be LTS which should mean that both Blob and web streams should be removed from experimental status (?).

edit: pretty sure this library also requires v16+ (or at least v15) to use AbortController anyways.

[KhafraDev]

EDIT: the reply was deleted

Why not use axios though?

I've never personally used axios, so I don't know what - if any - benefits it provides over a standardized fetch API. However, just looking it over I can point out a few things:

• For Node, Axios wraps around http(s) which means it's much slower than undici (see: https://github.com/nodejs/undici/#benchmarks)
• By using a fetch implementation, it means that the API is standardized and, once spec-compliant, unlikely to change in the future.
• I have no idea if axios can handle the weird, non-spec compliant @discordjs/formdata package (for that matter, node-fetch v3 removed this behavior as well). Undici provides an implementation for node without external dependencies.
• Undici is maintained by nodejs maintainers under the nodejs organization. It's possible that this might land in core, and there have been talks of doing so. If it lands in core, we can simply remove the const { fetch } = require('undici'); and it will work without changes.

[iCrawl]

Despite the comment above, I'm okay with the overall change once we know the stability will not be 1 in Node 16 LTS

[favna]

Why not use axios though?

Adding to the arguments given above

1. Axios also has a known bug in that you cannot send a body when sending a DELETE request.
2. Axios is currently still on 0.x which per strict semver means "initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable" (source). Therefore Axios should not be considered stable to begin with. Hardly a library we can afford to have in a project as large and with as many users as DiscordJS. I have been corrected, apparently Axios doesn't use semver but 0ver. If you treasure your sanity, do not open that URL.
3. Axios has a backlog of issues and open pull requests from here to Saturn. If, aside from point 1, we'd ever run into an issue then the likelihood of having to just fork it and publish @discordjs/axios would be extremely large because likewise the likelihood of them merging a PR and releasing that in a timely fashion is extremely small.

[advaith1]

From unidici's readme:

Specification Compliance

This section documents parts of the Fetch Standard which Undici does not support or does not fully implement.

Garbage Collection

• https://fetch.spec.whatwg.org/#garbage-collection

The Fetch Standard allows users to skip consuming the response body by relying on garbage collection to release connection resources. Undici does the same. However, garbage collection in Node is less aggressive and deterministic (due to the lack of clear idle periods that browser have through the rendering refresh rate) which means that leaving the release of connection resources to the garbage collector can lead to excessive connection usage, reduced performance (due to less connection re-use), and even stalls or deadlocks when running out of connections. Therefore, it is highly recommended to always either consume or cancel the response body.

// Do
const headers = await fetch(URL)
  .then(async res => {
    for await (const chunk of res) {
      // force consumption of body
    }
    return res.headers
  })

// Do not
const headers = await fetch(url)
  .then(res => res.headers)

will this be an issue?

[KhafraDev]

will this be an issue?

In some cases, I believe so:

• on ratelimits (429s)
• 500..599 status
• anything outside of 200..599

I can add in a utility to consume the body in these cases if you'd like.

edit: due to it being an issue with node's gc, wouldn't this also affect node-fetch as well?

[iShibi]

FormData#append takes a Blob as value. undici throws an error here, because file.file is not an instance of Blob.

discord.js/src/rest/APIRequest.js

Line 53 in 72e3958

if (file?.file) body.append(file.key ?? file.name, file.file, file.name);

[KhafraDev]

FormData#append takes a Blob as value. undici throws an error here, because file.file is not an instance of Blob.

discord.js/src/rest/APIRequest.js

Line 53 in 72e3958

if (file?.file) body.append(file.key ?? file.name, file.file, file.name);

Can file.file be anything other than a Buffer? From testing I haven't seen any other value. ty :)

[jimmywarting]

Can file.file be anything other than a Buffer? From testing I haven't seen any other value. ty :)

Using undici is really living on the edge of experimental apis...
undici is using latest stuff from NodeJS and requires the newest and experimental api's there is... which is still unstable

• Now undici is using require('node:buffer').Blob and not something else throughout their entire codebase (in fetch & FormData)
• undici created a own File class that extends the builtin NodeJS Blob
• And theirs FormData instance requires the input to be nothing but a instance of a NodeJS Blob coming from node:buffer

So no... you can't append a buffer into their FormData instance, you have to construct a File using there version of the File implementation. (or creating a Blob using node:buffer - that later gets upgraded by their FormData impl automatically according to spec)

About Blobs...

One of the reason why fetch-blob haven't switched to using the built in version of NodeJS Blob impl is b/c they are pretty darn useless unless you can't get them backed up by the filesystem somehow, otherwise they are just useless containers that stores a buffers in memory where you could as well use a uint8array or ArrayBuffer directly instead.

With fetch-blob you can get a File from the filesystem without holding anything in the memory.
something of which node's is looking into implementing nodejs/node#37340 But they are blocked by support for async blob sources nodejs/node#37338 and also implementing a File class nodejs/node#39015

Only then when File and blobs backed up by the file system would we over at node-fetch start to consider using the built in impl. and marking fetch-blob as legacy. cuz right now it serves a purpose not even NodeJS have fixed in their version.

About FormData

Now to the version of undici FormData impl... knowing what i have written earlier about node's wortless Blob container and how strict undici FormData only accept a version of NodeJS own Blob implementation...
Do you think undici dose a grate job at sending large multipart payloads with files embedded up to a few GB of files? the answer is No.
You are required to read all files, construct a new Blob (or a File that extends Blob) with buffers and it's all held in memory.

Unlike one other spec compatible FormData implementations such as formdata-polyfill it dose not use instanceof checks. It can work with other Blob/File implementations as long as they look like a Blob, so it can accept both a fetch-blob and a NodeJS blob

and it can generate a FormData boundary/payload without ever reading the files content.

undici is more focused on writing something they wish to see being implemented into core one day. not something that is best suitable for userland as they require the latest experimental features
undici is very new and not as much battle tested as node-fetch

if i would choose between undici and node-fetch then i would maybe go with node-fetch 2.x or 3.x b/c it's

• supported on more node's versions
• have been tested a lot by many users
• is less strict about what Blob, File, AbortSignal, URLSearchParams and FormData you are using. So it can work with files backed up by filesystem or other implementations cuz it dose not use instanceof checks instead uses duck typing to check if it looks like a duck and quack like a duck. This have been a feature of node-fetch since a very long beginning b/c nodejs lacks all those things.

[KhafraDev]

The issues you brought up are completely out of scope for this repo and mostly don't apply (either to the platform, usage, or users).

[iShibi]

The toBlob function doesn't work currently. The reason is that DataResolver#resolveFile returns a ReadableStream (Web Stream) for a URL, as that's what Response.body is in undici and a ReadStream (Nodejs Stream) for a local file path, as that's what fs#createReadStream returns.

Neither of them returns true for Buffer#isBuffer, hence they can't be converted into a Blob, which results in undici throwing as it expects a Blob as a value in FormDate#append.

I did some tinkering and got the FormData#append working which results in the bot correctly uploading the attachments. However, I don't know how efficient of an approach it is or what effects it has on the memory. Still, I'll write down what I did:

1. For a URL, we can directly return Response.blob() in DataResolver#resolveFile :

static async resolveFile(resource) {
  if (typeof resource === 'string') {
    if (/^https?:\/\//.test(resource)) {
      const res = await fetch(resource);
      // return res.body;
      return res.blob();
    }
    // ...
  }
}

Buffer#isBuffer will return false for Blob, hence toBlob will pass it as it is to FormData#append.

2. For a local file path, we can create a Buffer using fs.readFile:

static async resolveFile(resource) {
  if (typeof resource === 'string') {
    // ...
    return new Promise((resolve, reject) => {
      const file = path.resolve(resource);
      fs.stat(file, (err, stats) => {
        if (err) return reject(err);
        if (!stats.isFile()) return reject(new DiscordError('FILE_NOT_FOUND', file));
        return fs.readFile(file, (error, buf) => {
          if (error) return reject(error);
          return resolve(buf);
        });
      });
    });
  }
}

toBlob will then convert this Buffer into a Blob

[KhafraDev]

I got everything fixed & tested with both Buffer and node Readable streams - will push the changes in a few hours.

In resolveFile I ended up following your suggestions but I converted everything to blobs there and removed the toBlob function.

[iShibi]

Works fine. Make sure to update the typings too:

discord.js/typings/index.d.ts

Line 628 in fe95005

public static resolveFile(resource: BufferResolvable | Stream): Promise<Buffer | Stream>;

[KhafraDev]

I think this PR should be classified as semver-major due to 0618e7f

• DataResolver.resolveFileAsBuffer is no longer used (not removed however)
• DataResolver.resolveBase64 is now asynchronous (doesn't change behavior of the lib, but will affect anyone using it).
• DataResolver.resolveFile now accepts a Blob parameter (ie. MessageAttachment should support Blobs I believe).

[iCrawl]

This needs a rebase.

[vladfrangu]

Please fix lint

[KhafraDev]

Please fix lint

guess the precommit hook wasn't working, thought it would have caught that for me 😕

[KhafraDev]

@kyranet buffer.Blob still remains

[KhafraDev]

Although I believe v16 is about to be LTS which should mean that both Blob and web streams should be removed from experimental status (?).

I was slightly off with this one, by about 1 LTS version, but v18 will have a global Blob and experimental status removed as well :)
nodejs/node#41270

[KhafraDev]

hey 😏

[KhafraDev]

I have no interest in keeping this PR open as it likely won't land.

[ImRodry]

I know this was closed but undici's fetch just landed on node as an experimental flag, so now is an even better time to reopen this PR IMO

• lib: add fetch nodejs/node#41749

[suneettipirneni]

I know this was closed but undici's fetch just landed on node as an experimental flag, so now is an even better time to reopen this PR IMO

• lib: add fetch nodejs/node#41749

This was closed because it's going to be added to /rest now. Also there's a roadblock in mocking some of the request data.

[ImRodry]

I know this was closed but undici's fetch just landed on node as an experimental flag, so now is an even better time to reopen this PR IMO

• lib: add fetch nodejs/node#41749

This was closed because it's going to be added to /rest now. Also there's a roadblock in mocking some of the request data.

This was closed because the author didn't feel like keeping it up. There's no open issue about that to my knowledge

[suneettipirneni]

I know this was closed but undici's fetch just landed on node as an experimental flag, so now is an even better time to reopen this PR IMO

• lib: add fetch nodejs/node#41749

This was closed because it's going to be added to /rest now. Also there's a roadblock in mocking some of the request data.

This was closed because the author didn't feel like keeping it up. There's no open issue about that to my knowledge

I've spoken to the author and my reasons for why it's closed are from what they told me.

[ckohen]

I know this was closed but undici's fetch just landed on node as an experimental flag, so now is an even better time to reopen this PR IMO

How could you even reopen this PR? The majority of the point of the PR has been made obsolete because /rest has already replaced it.

As you've already been told, the author was told the switch to /rest was happening soon and thus this probably couldn't land. Between me and the author we are looking to make this PR a reality in /rest but switching the tests over is a major rewrite and since there are still things that are experimental in undici fetch we aren't rushing to do that.