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

Include typings for Typescript, for V2/V3 of Formidable #774

Open
gboer opened this issue Nov 2, 2021 · 14 comments
Open

Include typings for Typescript, for V2/V3 of Formidable #774

gboer opened this issue Nov 2, 2021 · 14 comments
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably be for additions or changes. Expected that this will result in a PR.

Comments

@gboer
Copy link

gboer commented Nov 2, 2021

Support plan

  • which support plan is this issue covered by?: Community
  • is this issue currently blocking your project?: Yes
  • is this issue affecting a production system?: Yes

Context

  • node version: v14.18.1
  • module (formidable) version: Any V2/V3
  • environment (e.g. node, browser, native, OS): NodeJS
  • used with (i.e. popular names of modules): Typescript 4.X
  • any other relevant information: N/A

What problem are you trying to solve?

I am trying to upgrade to Formidable V2 or V3, but since there are no typings available for this version, it is incredibly time-consuming to figure out how the API looks and what I need to place where. Since this project is downloaded millions of times per week, I can only assume that a lot of other people are running into the same issue. This makes adopting the new version unlikely, any time soon.

Do you have a new or modified API suggestion to solve the problem?

Include the typings with the new version.

@GrosSacASac
Copy link
Contributor

@karlhorky I saw you helped with types in the past, what should we do ?

@karlhorky
Copy link

Thanks for the ping! I also ran into this problem when upgrading (since the internals of v2 are incompatible with v1).

I fixed the problems that I encountered just as a patch (using patch-package in my local repository):

diff --git a/node_modules/@types/formidable/index.d.ts b/node_modules/@types/formidable/index.d.ts
index 249da05..e4b4e50 100755
--- a/node_modules/@types/formidable/index.d.ts
+++ b/node_modules/@types/formidable/index.d.ts
@@ -208,12 +208,12 @@ declare namespace formidable {
          * The path this file is being written to. You can modify this in the `'fileBegin'` event in case
          * you are unhappy with the way formidable generates a temporary path for your files.
          */
-        path: string;
+        filepath: string;
 
         /**
          * The name this file had according to the uploading client.
          */
-        name: string | null;
+        originalFilename: string | null;
 
         /**
          * The mime type of this file, according to the uploading client.

But of course this is not a solution for everyone... so continuing in the next comment 👇

@karlhorky
Copy link

karlhorky commented Nov 2, 2021

So I would ask you, @GrosSacASac and @tunnckoCore, would the maintainers of formidable be open to either of the following:

  1. Converting the project to TypeScript (or JSDoc + .d.ts files)
  2. Maintaining types as part of the formidable package

A less desirable alternative would be:

  1. Supporting the DefinitelyTyped community (the people who maintain @types/formidable) in PRs like fix(formidable): attempt to amend changes from #51364 DefinitelyTyped/DefinitelyTyped#52697

Oh and for people who are looking for a solution, @gboer has opened a PR to DefinitelyTyped to add the new types 👍 Thanks!

DefinitelyTyped/DefinitelyTyped#56925

@gboer
Copy link
Author

gboer commented Nov 2, 2021

hehe yeah, I was about to add that :) But there are no types for v3 yet and in the long run, it would still be nice if the typings would be delivered with formidable itself. Makes things a lot simpler for everyone.

@karlhorky
Copy link

karlhorky commented Nov 2, 2021

Yeah easiest long term and most beneficial would be if formidable@3 would be written in TypeScript.

@GrosSacASac
Copy link
Contributor

GrosSacASac commented Nov 2, 2021

Yes, as I said here #500 I would welcome if someone makes a PR to add types in formidable. If one of you wants I can also give github formidable access so you can make a branch instead of a fork to make a PR.

@tunnckoCore
Copy link
Member

yes, totally. vould be rewritten to typescript, I'm open for that.

@gboer
Copy link
Author

gboer commented Nov 5, 2021

small update, the types for Formidable V2 are now available in @types/[email protected] :)

@tunnckoCore
Copy link
Member

tunnckoCore commented Nov 6, 2021

@gboer great! :)

I'm for rewriting it to TypeScript with both hands, also moving to a monorepo #569. I didn't write a lot of code last year or more, so.. starting a rewrite in typescript could be incredibly tricky for me 🤣 me and typescript ... ah, it's hard relationship through the years hahaha

@tunnckoCore tunnckoCore added Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably be for additions or changes. Expected that this will result in a PR. and removed feature request labels Apr 2, 2022
@Akxe
Copy link
Collaborator

Akxe commented Apr 28, 2022

I will happily convert v3 to a typesafe version, but I could not find the branch 😁

PS: Found it, began the work 😊

@tunnckoCore
Copy link
Member

tunnckoCore commented Apr 29, 2022

@Akxe v3 is master already, you can start from there. I'll soon bump npm latest to v3 too.

@Akxe
Copy link
Collaborator

Akxe commented Apr 29, 2022

The typescript rewrite is quite hard without major changes to the codebase. There is a lot of Object.assign(this, ...) and a lot of sharing of the this "variable".

I would usually do either:

Parsers/plugins as standalone classes

that do not share anything with the Formidable/IncommingForm class

They would expose API that the Formidable would call and then either get a one-time result, a generator function, or even EventEmmiter back.

Parsers as an extension of Formidable

This is an option too, I did not finish looking through the plugin code in-depth yet.


Please before v3 is marked as latest edit README.MD with info, that you need to install v2 in order for webpack and probably similar to work.

@tunnckoCore
Copy link
Member

@Akxe they already are extensions/plugins.

There is a lot of Object.assign(this, ...) and a lot of sharing of the this "variable".

No problemo. The this use was intentional, but they work without it too.

Please before v3 is marked

Good point.

@wbt
Copy link
Collaborator

wbt commented Apr 30, 2022

You may want to merge this before treating anything from the types repo as a starting point, FYI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Status: In Progress This issue is being worked on, and has someone assigned. Type: Enhancement Most issues will probably be for additions or changes. Expected that this will result in a PR.
Projects
None yet
Development

No branches or pull requests

6 participants