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

WIP: TypeScript #899

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

tommyhtran
Copy link

This PR aims to convert the source code into TypeScript without major refactoring. Benchmarks, examples, and tests are excluded from the scope of this PR.

I'm aware of #854 already open, but development on it appears to have stalled. This is simply my attempt at converting this project.

This PR addresses #774 and #830.

My test results are at parity with commit 493ec88 but with one exception due to two tests using the same port.

 FAIL  test/standalone/connection-aborted.test.js (6.828 s)
  × connection aborted (5010 ms)

  ● connection aborted

    listen EADDRINUSE: address already in use 127.0.0.1:13539
Test Suites: 6 failed, 10 passed, 16 total
Tests:       17 failed, 3 skipped, 57 passed, 77 total
Snapshots:   0 total
Time:        8.456 s
Ran all test suites.

strictNullChecks has been disabled and is pending future refactoring.

The following errors are currently reported by the TypeScript compiler:

open/close
src/Formidable.ts:9:20 - error TS2792: Cannot find module 'hexoid'. Did you mean to set the 'moduleResolution' option to 'node', or to add aliases to the 'paths' option?

9 import hexoid from 'hexoid';
                     ~~~~~~~~

src/Formidable.ts:11:21 - error TS2792: Cannot find module 'dezalgo'. Did you mean to set the 'moduleResolution' option to 'node', or to add aliases to the 'paths' option?

11 import dezalgo from 'dezalgo';
                       ~~~~~~~~~

src/Formidable.ts:229:24 - error TS2339: Property 'end' does not exist on type 'OctetStreamParser | JSONParser | DummyParser | (typeof MultipartParser & { STATES: {}; }) | QuerystringParser'.
  Property 'end' does not exist on type 'typeof MultipartParser & { STATES: {}; }'.

229           this._parser.end();
                           ~~~

src/Formidable.ts:252:18 - error TS2349: This expression is not callable.
  Each member of the union type '{ (event: "close", listener: () => void): OctetStreamParser; (event: "data", listener: (chunk: any) => void): OctetStreamParser; (event: "end", listener: () => void): OctetStreamParser; (event: "error", listener: (err: Error) => void): OctetStreamParser; (event: "pause", listener: () => void): OctetStreamParser; (ev...' has signatures, but none of those signatures are compatible with each other.

252     this._parser.once('error', (error: Error) => {
                     ~~~~

src/Formidable.ts:271:18 - error TS2339: Property 'write' does not exist on type 'OctetStreamParser | JSONParser | DummyParser | (typeof MultipartParser & { STATES: {}; }) | QuerystringParser'.
  Property 'write' does not exist on type 'typeof MultipartParser & { STATES: {}; }'.

271     this._parser.write(buffer);
                     ~~~~~

src/Formidable.ts:305:9 - error TS2345: Argument of type 'BufferEncoding | "7bit" | "8bit"' is not assignable to parameter of type 'BufferEncoding'.
  Type '"7bit"' is not assignable to type 'BufferEncoding'.

305         part.transferEncoding || this.options.encoding,
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/helpers/firstValues.ts:5:40 - error TS7006: Parameter 'fields' implicitly has an 'any' type.

5 const firstValues = (form: Formidable, fields, exceptions: string[] = []) => {
                                         ~~~~~~

src/helpers/firstValues.ts:15:20 - error TS7053: Element implicitly has an 'any' type because expression of type '0' can't be used to index type '{}'.
  Property '0' does not exist on type '{}'.

15       return [key, value[0]];
                      ~~~~~~~~

src/helpers/readBooleans.ts:1:23 - error TS7006: Parameter 'fields' implicitly has an 'any' type.

1 const readBooleans = (fields, listOfBooleans: string[]) => {
                        ~~~~~~

src/index.ts:9:21 - error TS7019: Rest parameter 'args' implicitly has an 'any[]' type.

9 const formidable = (...args) => new Formidable(...args);
                      ~~~~~~~

src/parsers/JSON.ts:26:16 - error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'Error'.
  Type '{}' is missing the following properties from type 'Error': name, message

26       callback(e);
                  ~

src/parsers/Multipart.ts:39:16 - error TS7006: Parameter 'c' implicitly has an 'any' type.

39 function lower(c) {
                  ~

src/parsers/Multipart.ts:46:3 - error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{}'.
  No index signature with a parameter of type 'string' was found on type '{}'.

46   STATES[stateName] = STATE[stateName];
     ~~~~~~~~~~~~~~~~~

src/parsers/Multipart.ts:46:23 - error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ PARSER_UNINITIALIZED: number; START: number; START_BOUNDARY: number; HEADER_FIELD_START: number; HEADER_FIELD: number; HEADER_VALUE_START: number; HEADER_VALUE: number; HEADER_VALUE_ALMOST_DONE: number; ... 4 more ...; END: number; }'.
  No index signature with a parameter of type 'string' was found on type '{ PARSER_UNINITIALIZED: number; START: number; START_BOUNDARY: number; HEADER_FIELD_START: number; HEADER_FIELD: number; HEADER_VALUE_START: number; HEADER_VALUE: number; HEADER_VALUE_ALMOST_DONE: number; ... 4 more ...; END: number; }'.

46   STATES[stateName] = STATE[stateName];
                         ~~~~~~~~~~~~~~~~

src/parsers/Multipart.ts:123:7 - error TS7053: Element implicitly has an 'any' type because expression of type '`${string}Mark`' can't be used to index type 'MultipartParser'.

123       this[`${name}Mark`] = typeof idx === 'number' ? idx : i;
          ~~~~~~~~~~~~~~~~~~~

src/parsers/Multipart.ts:127:14 - error TS7053: Element implicitly has an 'any' type because expression of type '`${string}Mark`' can't be used to index type 'MultipartParser'.

127       delete this[`${name}Mark`];
                 ~~~~~~~~~~~~~~~~~~~

src/parsers/Multipart.ts:137:44 - error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'MultipartParser'.
  No index signature with a parameter of type 'string' was found on type 'MultipartParser'.

137         this._handleCallback(name, buffer, this[markSymbol], buffer.length);
                                               ~~~~~~~~~~~~~~~~

src/parsers/Multipart.ts:140:44 - error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'MultipartParser'.
  No index signature with a parameter of type 'string' was found on type 'MultipartParser'.

140         this._handleCallback(name, buffer, this[markSymbol], i);
                                               ~~~~~~~~~~~~~~~~

src/parsers/Multipart.ts:341:39 - error TS2339: Property 'stateToString' does not exist on type 'typeof MultipartParser'.

341     return `state = ${MultipartParser.stateToString(this.state)}`;
                                          ~~~~~~~~~~~~~

src/parsers/Multipart.ts:346:17 - error TS2339: Property 'stateToString' does not exist on type 'typeof MultipartParser'.

346 MultipartParser.stateToString = (stateNumber: number) => {
                    ~~~~~~~~~~~~~

src/parsers/Multipart.ts:348:20 - error TS7053: Element implicitly has an 'any' type because expression of type 'string' can't be used to index type '{ PARSER_UNINITIALIZED: number; START: number; START_BOUNDARY: number; HEADER_FIELD_START: number; HEADER_FIELD: number; HEADER_VALUE_START: number; HEADER_VALUE: number; HEADER_VALUE_ALMOST_DONE: number; ... 4 more ...; END: number; }'.
  No index signature with a parameter of type 'string' was found on type '{ PARSER_UNINITIALIZED: number; START: number; START_BOUNDARY: number; HEADER_FIELD_START: number; HEADER_FIELD: number; HEADER_VALUE_START: number; HEADER_VALUE: number; HEADER_VALUE_ALMOST_DONE: number; ... 4 more ...; END: number; }'.

348     const number = STATE[stateName];
                       ~~~~~~~~~~~~~~~~

src/PersistentFile.ts:73:27 - error TS2339: Property 'closed' does not exist on type 'Writable'.

73     if (this._writeStream.closed) {
                             ~~~~~~

src/plugins/multipart.ts:25:29 - error TS2684: The 'this' context of type 'void' is not assignable to method's 'this' of type 'IncomingForm'.

25       const initMultipart = createInitMultipart(m[1] || m[2]);
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/plugins/multipart.ts:80:9 - error TS2322: Type 'string' is not assignable to type 'string[]'.

80         part.headers[headerField] = headerValue;
           ~~~~~~~~~~~~~~~~~~~~~~~~~

src/plugins/multipart.ts:160:41 - error TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.

160                 const offset = parseInt(part.transferBuffer.length / 4, 10) * 4;
                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/plugins/octetstream.ts:61:22 - error TS2339: Property 'emit' does not exist on type 'OctetStreamParser | JSONParser | DummyParser | (typeof MultipartParser & { STATES: {}; }) | QuerystringParser'.
  Property 'emit' does not exist on type 'typeof MultipartParser & { STATES: {}; }'.

61         this._parser.emit('doneWritingFile');
                        ~~~~

src/plugins/octetstream.ts:80:20 - error TS2349: This expression is not callable.
  Each member of the union type '{ (event: "close", listener: () => void): OctetStreamParser; (event: "data", listener: (chunk: any) => void): OctetStreamParser; (event: "end", listener: () => void): OctetStreamParser; (event: "error", listener: (err: Error) => void): OctetStreamParser; (event: "pause", listener: () => void): OctetStreamParser; (ev...' has signatures, but none of those signatures are compatible with each other.

80       this._parser.once('doneWritingFile', done);
                      ~~~~

src/VolatileFile.ts:76:27 - error TS2339: Property 'closed' does not exist on type 'Writable'.

76     if (this._writeStream.closed || this._writeStream.destroyed) {
                             ~~~~~~


Found 28 errors.

I would appreciate any help with fixing these errors.

@GrosSacASac
Copy link
Contributor

Thanks , I cannot look right now

@tommyhtran
Copy link
Author

I've gotten TypeScript errors down to the following:

src/Formidable.ts:305:9 - error TS2345: Argument of type 'BufferEncoding | "7bit" | "8bit"' is not assignable to parameter of type 'BufferEncoding'.
  Type '"7bit"' is not assignable to type 'BufferEncoding'.

305         part.transferEncoding || this.options.encoding,
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/plugins/multipart.ts:24:29 - error TS2684: The 'this' context of type 'void' is not assignable to method's 'this' of type 'IncomingForm'.

24       const initMultipart = createInitMultipart(m[1] || m[2]);
                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/plugins/multipart.ts:109:11 - error TS2322: Type 'string' is not assignable to type 'BufferEncoding | "7bit" | "8bit"'.

109           part.transferEncoding = headerValue.toLowerCase();
              ~~~~~~~~~~~~~~~~~~~~~

src/plugins/multipart.ts:149:41 - error TS2345: Argument of type 'number' is not assignable to parameter of type 'string'.

149                 const offset = parseInt(part.transferBuffer.length / 4, 10) * 4;
                                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 4 errors.

There are two things that should probably be addressed in a separate PR.

  1. const offset = parseInt(part.transferBuffer.length / 4, 10) * 4;

    A number value is passed to parseInt() when it should be a string value. I believe the proper fix is to call Math.floor() instead of parseInt().
  2. case '7bit':
    case '8bit':

    Formidable accepts 7bit and 8bit values in the HTTP Content-Transfer-Encoding header. These values eventually get passed into a StringDecoder (
    const decoder = new StringDecoder(
    part.transferEncoding || this.options.encoding,
    );
    ), but Node does not recognize these two values(see https://nodejs.org/api/buffer.html#buffers-and-character-encodings). Should 7bit be mapped to ascii and 8bit be mapped to latin1?

@tunnckoCore
Copy link
Member

tunnckoCore commented Nov 22, 2022

@tommyhtran hey! Thanks for your work on this front. I'm not in friendly relationships with typescript 😆 But yea.

I can't help much except the few things. I really start hating and rage-quit the editor and coding for some time when I got to some more deep TS codebase. 🤣 🤣 Not sure how I'll feel here too 😆

As about 1), we can convert it to parseInt(String(part.transferBuffer.length / 4), 10) or yea probably Math.floor.

As about the second, I really am not familiar with 7-8bits thing. It's there just because legacy reasons :D
Maybe better would be to just drop it and see if someone is even using it. From the docs there seems like that yes 7bit is ascii. Maybe if you look at the v0.8 - 0.10 docs... ahhahaha.

Jokes aside, what you're saying looks about right at first glance.

But also... the fact that it was there for so many years, and no one even tried (because we wouldn't be able) to use ascii or latin1 shows that it isn't even needed. I think?

I'll review it at a later point, but I don't think a review is much needed. Maybe the only thing that I could ask is to try with separate file approach - e.g. just a typings file that we will distribute with the package. Just to see how it looks like.

@hyperupcall
Copy link

Another approach is to not change the files to TypeScript and just add typings via JSDoc. And as said before, use a separate typings file for the more complicated types.

@hichemfantar
Copy link

Is this still WIP or is it abandoned?

@GrosSacASac
Copy link
Contributor

@hichemfantar see comment to vote #830 (comment)

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

Successfully merging this pull request may close these issues.

None yet

5 participants