Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeScript dom lib FormData and File type compatibility #57

Open
jaydenseric opened this issue Jul 7, 2022 · 11 comments
Open

TypeScript dom lib FormData and File type compatibility #57

jaydenseric opened this issue Jul 7, 2022 · 11 comments

Comments

@jaydenseric
Copy link
Contributor

jaydenseric commented Jul 7, 2022

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

octet-stream commented Jul 8, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

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

@jaydenseric
Copy link
Contributor Author

jaydenseric commented Jul 8, 2022

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
Copy link
Owner

octet-stream commented Jul 8, 2022

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
Copy link
Owner

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: microsoft/TypeScript#29867

@octet-stream
Copy link
Owner

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

@jimmywarting
Copy link
Contributor

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

@octet-stream
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants