octet-stream / form-data

Spec-compliant FormData implementation for Node.js
https://www.npmjs.com/package/formdata-node
MIT License
142 stars 17 forks source link

TypeScript `dom` lib `FormData` and `File` type compatibility #57

Open jaydenseric opened 2 years ago

jaydenseric commented 2 years ago

It would be great if the FormData class exported by this package was compatible with the TypeScript dom lib FormData type. The same goes (I guess) for the File class.

This would allow libraries to specify FormData type from the TypeScript dom lib for function arguments, etc. and not cause type problems for projects that are using FormData instances from this package with that.

Screen Shot 2022-07-08 at 9 38 54 am

It looks like the File class needs to have a webkitRelativePath property to be complaint.

MDN links to this spec for the property:

https://wicg.github.io/entries-api/#dom-file-webkitrelativepath

Given that the TypeScript dom lib types don't have webkitRelativePath as an optional type, it must mean it's implemented by all browsers?

For context, I am preparing a big apollo-upload-client update that (among other things) adds type safety and am wondering how to type the function createUploadLink option FormData which allows users to customise the FormData class used; the first thing I'm trying is typeof FormData which references the TypeScript dom lib type, but then in tests it results in a type error when I try to use FormData from this package with that option.

octet-stream commented 2 years ago

Hey, good catch, I have never really tested types compatibility. You're right, this method is implemented by all browsers, according to compatibility table on MDN page of this method: https://developer.mozilla.org/en-US/docs/Web/API/File/webkitRelativePath#browser_compatibility

I tried to fix this, but it introduces different errors (with ReadableStream), so I will continue attempt to improve compatibility later this day, because it's late night here, sorry.

image
octet-stream commented 2 years ago

Tried in fresh setup with the code from latest commit and it does seem to work now, I don't get any errors. For more input:

tsconfig.json:

{
  "include": ["test.mjs"],
  "exclude": ["node_modules"],
  "compilerOptions": {
    "allowJs": true,
    "checkJs": true,
    "noEmit": true,
    "target": "esnext",
    "baseUrl": ".",
    "moduleResolution": "node"
  }
}

test.mjs

import {FormData as FormDataNode} from "formdata-node"

/** @type {FormData} */
const form = new FormDataNode()

form.set("hello", "Hello, World!")

Can you confirm? Just install my package right from github, it should be compiled right away.

octet-stream commented 2 years ago

Not sure why the error mentioned in my first comment is happening, but I assume TS falls back to NodeJS.ReadableStream interface, if web streams does not have [Symbol.asyncIterator](). Got this error when I tried to test your issue right in this project and then when I set up the fresh installation, but without tsconfig.json

jaydenseric commented 2 years ago

I tried using your master branch in the project, and it had this error:

Screen Shot 2022-07-09 at 7 25 30 am Screen Shot 2022-07-09 at 7 27 26 am

As a side note, I recommend replacing "moduleResolution": "node" in your TypeScript config with "module": "nodenext". That way it will take into account modern Node.js stuff like the package exports field, .mjs and ESM rules about import paths and file extensions, etc.

octet-stream commented 2 years ago

Do you have tsconfig.json in your project? I don't see this error when I have one.

jaydenseric commented 2 years ago

I use a jsconfig.json (which is like a tsconfig.json but it automatically enables allowJs):

{
  "compilerOptions": {
    "maxNodeModuleJsDepth": 10,
    "module": "nodenext",
    "noEmit": true,
    "strict": true
  },
  "typeAcquisition": {
    "enable": false
  }
}

With this config you need to add a // @ts-check comment at the top of files you want TypeScript to check.

octet-stream commented 2 years ago

With this config you need to add a // @ts-check comment at the top of files you want TypeScript to check.

You don't if you set checkJs to true. Works with jsconfig.json

image

With jsconfig.json I see this error, but not with tsconfig.json. Interesting.

octet-stream commented 1 year ago

While working on v6 I found out that types for global ReadableStream have no Symbol.asyncIterable which makes it incompatible with undici BodyInit, because it relies on Node.js' types for Blob, which has Symbol.asyncIteralbe

image

Then if I would use ReadableStream class from node:streams/web - I'll get an error for global BodyInit, I think it because now ReadalbeStream becomes incompatible with BodyInit, since it's the only thing I change.

image

Weird. I'm not sure what to do, but I probably will choose to support types for global BodyInit, because eventually ReadableStream should get Symbol.asyncIterable.

Maybe I'm missing something and there's already typings for ReadableStream with Symbol.asyncIterable, but I didn't see lib option for those too in tsconfig.

I think this could probably be related: https://github.com/microsoft/TypeScript/issues/29867

octet-stream commented 1 year ago

v6 is available now, check out release notes for more information: https://github.com/octet-stream/form-data/releases/tag/v6.0.0

jimmywarting commented 1 year ago

imo i think typescript should remove this non speced webkitRelativePath... ppl should not depend on it...

octet-stream commented 1 year ago

Great, I just found out that on the type-level my Blob implementation is not compatible to either Node.js' one, to node-fetch, and undici. And that's just because I have Symbol.toStringTag (which is present on File, FormData and Blob in browsers, as well as in many other object from what I can see), but none of those have one. I should've tested this better before releasing v6, but I completely forgot! And making it optional (as I did in the form-data-encoder for FileLike and FormDataLike) would technically be a breaking change, because string returned by this property will become a string | undefined, plus the property itself will be an optional. I should probably bring this to TS team, but it might take awhile until this will be resolved on their side.