Rewrite to Typescript

[Akxe]

This is work in progress

not to be merged yet!

Formidable (IncommingForm)

• Done?

My main concern here is that a log of variables is not defined before the parse method is called. From my understanding, the idea is that one Formidable instance can be parsing one request at a time, but can be reused after it has completed reading the form. (Why is it that it can be reused, what is the point?)

I would propose, that the parse would return a subclass, that would only live until the parsing is complete, then it would be a frozen object/class instance. The parse method then could:

• Accept new parameters like expected booleans that would automatically convert and/or add missing boolean values ("on" and "true" to true and the rest to false).
• Accept the JSON reviver function

Plugins

• Done?

I still have more to read through.

Parsers

• Done?

I still have more to read through.

Misc

• Files - I have made them a common class they both extend from, and unified most of the API. This might be a breaking change!
• Helpers - How about replacing them with the subclass from Formidable.parse? It could return a Promise<ParsedForm>, where ParsedForm is:

class ParsedForm<
  // Setting this is unsafe, but is normal practice to allow developers to defined expected output
  Fields extends Record<string, string | string[]> | string[],
  Files extends Record<string, FormidableFile | FormidableFile[]>
>{
  readonly files: Files;
  readonly fields: Fields;

  readFirstOf<K extends keyof Files>(key: K): Files<K>;
  readFirstOf<K extends keyof Fields>(key: K): Fileds<K>;
}

[Akxe]

I have a few questions for now:

• Formidable.js:409 - variable results is created and returned via emitter from this function, but it is never referenced and thus could not be filled with data.
• How does a plugin return anything? How does the binding of class into self work?
• Formidable.js:548 - This line is rightfully highlighted with the error This condition will always return true since this function is always defined. Did you mean to call it instead?. The function that is being tested for existence is the same as the one that is being called and the function is statically defined, thus it should never be missing either.

[GrosSacASac]

Converting to Typescript is already a huge task. You refactoring the code at the same time reduces the likelihood of this PR being merged fast. Because it creates opportunity for disagreement.

[GrosSacASac]

• Correct ,

  formidable/src/Formidable.js

  Line 442 in 64f32c2

  results.push(pluginReturn);

  add this line back
• A plugin does not really return , instead it attaches properties to the formidable instance (this._parser = parser;)

[GrosSacASac]

I don't know where but if it is always true then maybe it is a leftover from before a refactor where it wasn't the case

[GrosSacASac]

Maybe in a future PR we will make .parse return a Promise so try to not change too much in this PR

[Akxe]

• Correct ,

  formidable/src/Formidable.js

  Line 442 in 64f32c2

  results.push(pluginReturn);

  
  add this line back
• A plugin does not really return , instead it attaches properties to the formidable instance (this._parser = parser;)

My point is, that since the plugin does not return (only modifies this), and the results variable is not used it is redundant and should be removed.

[Akxe]

Converting to Typescript is already a huge task. You refactoring the code at the same time reduces the likelihood of this PR being merged fast. Because it creates opportunity for disagreement.

You are right, rewrite to TS is huge in itself, but in order to make usage of TS sensible, some parts might need to be rewritten.

[Akxe]

Maybe in a future PR we will make .parse return a Promise so try to not change too much in this PR

Well, the point was not to make it a promise. The current implementation of the parse function has a critical flaw.

Take the code below:

// Create parser (mistake but a sensible assumption to make
const form = formidable({});
const server = http.createServer((req, res) => {
  if (req.url === '/api/upload' && req.method.toLowerCase() === 'post') {
    form.parse(req, (err, fields, files) => {
      // Process result
    });

    return;
  }
});

Since the form will clean up itself after the request is parsed, one could assume, that creating a global form with common options is the right way and it would work most of the time. But once 2 requests come at the same time:

• Parse will accept the request and save it internally along with a callback and some more variables.
• It will start the first part of the stream synchronously
• Then once the first async thing comes, the second request can come in and override defined variables and since the this variable is shared, it will create a total hawoc.

---

This could be fixed using a simple if statement in parse, or it could be fixed by making the parse create a new instance of parsing entity.

This would also make the IncommingForm initialization class from:

export class IncomingForm<
  // Setting this is unsafe, but is normal practice to allow developers to define expected output
  Fields extends Record<string, string | string[]> | string[],
  Files extends Record<string, FormidableFile | FormidableFile[]>
> extends EventEmitter {
  readonly options: FormidableOptions<Fields, Files>;

  error?: Error;
  headers?: IncomingHttpHeaders;
  type?: string;
  ended = false;
  bytesExpected = 0;
  bytesReceived = 0;
  readonly openedFiles: FormidableFile[] = [];

  private req?: IncomingMessage;
  private parser: undefined;
  private flushing = 0;
  private fieldsSize = 0;
  private totalFileSize = 0;
  private plugins: Transform[] = [];
}

into

export class IncomingForm<
  // Setting this is unsafe, but is normal practice to allow developers to define expected output
  Fields extends Record<string, string | string[]> | string[],
  Files extends Record<string, FormidableFile | FormidableFile[]>
> extends EventEmitter {
  readonly options: FormidableOptions<Fields, Files>;

  private readonly headers = this.getHeaders(this.req);
  private readonly type = this.getType(this.headers);
  // ended = false;  // Redundant, once it is done, the parse method returns and all cease activity, can be kept thought...
  // error?: Error; // Same as above
  private readonly bytesExpected = this.getExpectedBytes(this.headers);
  bytesReceived = 0;
  readonly openedFiles: FormidableFile[] = [];

  private flushing = 0;
  private fieldsSize = 0;
  private totalFileSize = 0;
  private plugins: Transform[] = [];
  constructor(
    private readonly req: IncomingMessage,
  ) {}
}