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

Rewrite to Typescript #854

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

Rewrite to Typescript #854

wants to merge 3 commits into from

Conversation

Akxe
Copy link
Collaborator

@Akxe Akxe commented Apr 30, 2022

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 Akxe changed the title Typescript Rewrite to Typescript Apr 30, 2022
@Akxe
Copy link
Collaborator Author

Akxe commented Apr 30, 2022

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

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

  • Correct ,
    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
Copy link
Contributor

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

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

@Akxe
Copy link
Collaborator Author

Akxe commented May 2, 2022

  • Correct ,
    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
Copy link
Collaborator Author

Akxe commented May 2, 2022

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.

@wbt wbt marked this pull request as draft May 2, 2022 17:48
@Akxe
Copy link
Collaborator Author

Akxe commented May 2, 2022

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,
  ) {}
}

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.

2 participants