RFC: Simplify and standardise object instance checking by bringing them into this organisation

[tinovyatkin]

This project has quite a long history of searching the correct way for checking non-native Node objects to spy whether they are an instance of some particular classes we use in node-fetch. There are objects that we are trying to identify:

1. URLSearchParams
2. Blob / File
3. AbortController / AbortSignal
4. FormData

The project went from an initial approach of using instanceof whether it was possible to current duck typing check combined with checking on Symbol.toStringTag for an exact match. And we are receiving a lot of critique for such an approach (#751, #848, #784, #667, #613, #296 to name a recent few).

However, if we will look at these objects then it's easy to notice that URLSearchParams is landed in Node.js core since version 10, and we have already implemented Blob within this organization and serialization logic for FormData (most complicated part of supporting this format) at node-fetch core (#603).

So, my proposal is to bring reference and lightweight implementations of AbortController and FormData (without serialization is pretty small, see #876 (comment)) into this organization (maybe publishing as @fetch/abort-controller, @fetch/form-data) and standardize checking at instanceof.

instanceof being a language standard way for checking instances is also very extensible:

• on a base class implementation, we can extend it using Symbol.hasInstance (see use Symbol.hasInstance fetch-blob#46)
• on an existing implementation, it can be spoofed by a Proxy (see More permissive isStream logic #848 (comment))

Both techniques are easy to document and should prevent any further discussions/critiques on it.

Doing so will allow us to drop legacy code from node-fetch and set a documented way for extending the library.

---

For references, current implementations:

1. abort-controller - written in TypeScript, well tested (94% code coverage), but has some browser-specific code that may be omitted

2. abortcontroller-polyfill - again, targetted to browsers, so, maybe greatly optimized/reduced for Node-only environment.

3. node-abort-controller - small, based on EventEmitter and targeting Node-only. My proposal is to invite @southpolesteve to donate this repository into the @node-fetch organization or create our own in the base of a similar approach. TypeScript declaration linked to lib.dom so should be rewritten.

4. FormData - @jimmywarting polyfill of FormData targeting browsers.

5. formdata-node - targeting Node.JS but contains serialization code that @octet-stream mostly implemented in the core of node-fetch. It also includes its own implementation of Blob/File that should be replaced by ours.

6. form-data - original implementation of FormData for Node, contains serialization logic that we don't need, not the spec-compliant.

There are quite a few cases with popular open-sources projects that did the same way - initially coupled with 3rd party modules maintained by individual authors for some important part of their functionality they decided to move/fork those modules into their organizations to guarantee maintainability and future-proof of main libraries. Some notable examples here are cssstyle at @jsdom and koa-router at @koajs

/cc @node-fetch/core @node-fetch/reviewers

[xxczaki]

I highly endorse this idea 👍 In terms of publishing the packages - we own the node-fetch organization on npm, so we can publish the implementations there 😄

[larson-carter]

This is a great idea 💪 We just need to figure out how we can execute it.

[bitinn]

I like the idea of it, though unless @tinovyatkin you are payed to do this full-time, perhaps doing them one by one is more attainable; I want to avoid node-fetch bringing in every part of Fetch ecosystem, but then let them fall into disrepair due to shortage of maintainer.

In many ways I appreciate the open ecosystem of node.js, even though it means sometimes a library doesn't quite deliver exactly what you want (or have more features than you need).

So figure out some real-world pain points (from their repo issues page perhaps) and see if there are actual drive to maintain them actively, if so, we can bring them in.

[jimmywarting]

I want to avoid node-fetch bringing in every part of Fetch ecosystem,

In many ways I appreciate the open ecosystem of node.js, even though it means sometimes a library doesn't quite deliver exactly what you want (or have more features than you need).

Same here. It's nice when libraries are decoupled and can be used in other libs as well without having to depend on the hole fetch ecosystem. Like how Axios could benefit from FormData without needing any of the fetch related stuff.

It would also be nice if some parts could be optional dependencies. I might not even need the Blob, FormData or base64 data URI support at all - in one project i started to use node-fetch@v3 beta 3 or 4 but later uninstalled it since it was a bit troublesome with import.
I only needed it for one thing to send out email verification / forgot psw to a third party provider via some API. I later ditched the hole node-fetch and just used simple core https module from node since it felt like a overburden.

At some point i don't agree with what some package are writing in there description

A light-weight module that brings...`

once it exceeds a certain size then i don't agree with them anymore and i start to write the necessary code myself at some point. sure there package maybe is small but if you take a look at all dependencies, then it's another thing.

---

but i agree that it would be nice to have more stuff under our organisation too. but at the same time i hate to see competing / duplicated similar packages with one small differences. it can be so annoying when i depend on two package and each one of them use on different version of FormData like form-data + formdata-node

---

Can't help but feel like it would be better if stuff lived on the globalScope instead and node-fetch could just use globalThis.Blob or globalThis.FormData when they exist. or throw an error 'not implemented/supported - install formdata-node'

[Richienb]

I believe the destiny of node-fetch is to have fetch in the Node.js core so that this module wouldn't be necessary anymore. The only reason why its not there is because it's insanely difficult to replicate the spec. It would need to have web streams, AbortController, FormData and Blob. However, this is what we're trying to do here. You can take this either way.

[octet-stream]

Speaking of form-data encoding: I made a package that basically moves formdata-node serialization algorithm out of my FormData implementation, so it can be used with different libraries to perform the encoding in different HTTP clients: https://github.com/octet-stream/form-data-encoder

It's on early stage (I just released version 0.1.0), but it already works. I've tested it with both formdata-node and formdata-polyfill.

[jimmywarting]

Funny enough, i did the same thing with my package. only differences is, I convert the formdata into a Blob in just 20 lines of code.

import { FormData, formDataToBlob } from 'formdata-polyfill/esm.min.js'
import formDataToBlob from 'formdata-polyfill/formdata-to-blob.js'
import formDataToBlob from 'https://github.com/jimmywarting/FormData/blob/master/formdata-to-blob.js'

const blob = formDataToBlob(new FormData())

Think a blob is easier to post since it don't involve setting any headers or iterable from stream + it don't have to calculate strings byte length or depend on any other node packages or built in functions such as Buffer you get all of the other benefits from blobs (like blob type and size) so in a essences this is more cross platform compatible for other environments

[octet-stream]

Nothing holds me back from making this module targeting more than just Node and stop using its specific APIs, such as Buffer or crypto. And I do believe that async iterable approach is better over the Blob because it doesn't require some specific class from a browser environment in a first place. It also will start the encoding process once you start to iterate through the Encoder instance, when Blob requires to copy FormData entries to an array before you create a Blob (You can, however, write a Blob-ish class that will handle encoding differently, but it will require HTTP client for less restricted Blob checking, as far as I can tell). You can write a small wrapper to put this encoding into ReadableStream, Node.js Readable stream or even into Blob as you did. But you don't really have to put the Encoder into Readable if client supports it or at least async iterables as request body.
On other hand, yes, it does require you to set headers if your HTTP client does not support this encoder, which I think is not a big deal at all.

[jimmywarting]

Blob requires to copy FormData entries to an array before you create a Blob

true, i don't consider copying the entries into a new array is a biggie, formdata are usually very short, and blob/file entries don't copy the data either (they are just referenced) it will also be closer to the data type that is needed to send the data - you have to decode the string entries values twice (once when computing the bytelength and the 2nd time when call the encode() function)

having a blob have allowed me to cache and reuse the instance in more ways than just posting. They can also easily be download with a[download] + that browser don't support streaming uploads yet (that's the historical main reason for not using a encoder in the first place) (so using toBlob at least in the browser is better)

---

Think i spottet something potentially wrong in your code
the chunks could be Uint8Arrays (i think - at least with the newer fetch-blob that no longer depends on Buffer anymore)

• Buffer.isBuffer(new Uint8Array) - false
• Buffer.from('') instanceOf Uint8Array - true

  async* encode(): AsyncGenerator<Buffer, void, undefined> {
    for await (const chunk of this._getField()) {
      yield Buffer.isBuffer(chunk) ? chunk : Buffer.from(String(chunk))
    }
  }

Maybe you should make this change?

  async* encode(): AsyncGenerator<Buffer, void, undefined> {
    for await (const chunk of this._getField()) {
-      yield Buffer.isBuffer(chunk) ? chunk : Buffer.from(String(chunk))
+      yield typeof chunk === 'string' ? Buffer.from(chunk) : chunk
    }
  }

[octet-stream]

I'm planning to get rid of Buffer in this library anyway and move forward to Uint8Array and maybe TextEncoder (I believe it is the way to encode text into the Uint8Array in browsers?), so yeah I will definitely change these lines and check for compatibility with fetch-blob, web streams etc

[octet-stream]

Not sure though if I can run fetch-blob in my tests, because it's ESM only and my module will target both ESM and CJS until I drop Node 12 next year. Same way for formdata-node though. As far as I can tell, I can use import() to require ESM modules in CJS context, so maybe I'll figure this out and add some more compatibility tests. I hope so.

[jimmywarting]

I'm planning to get rid of Buffer in this library anyway and move forward to Uint8Array and maybe TextEncoder (I believe it is the way to encode text into the Uint8Array in browsers?)

👍 That's correct. TextEncoder is the way to go if you don't want/need to use the Buffer

[octet-stream]

Well, this one is done now.

---

The other news is that it seems like I'm stuck with TypeScript not being able to not transpile import() expressions into Promise.resolve().then(() => require()), so maybe I will be able to test fetch-blob compatibility only when I drop CJS at all if they don't manage to find the solution for this problem within the year. I hope they will.

[octet-stream]

@jimmywarting does beta version of node-fetch support async iterables? I'm running into a problem with this and I should probably open an issue in fetch-blob or node-fetch...