Open Akxe opened 2 years ago
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.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.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.
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
Maybe in a future PR we will make .parse return a Promise so try to not change too much in this PR
- Correct , https://github.com/node-formidable/formidable/blob/64f32c2d5486918b2b8afdc2c6cc994d5f9a4a61/src/Formidable.js#L442 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.
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.
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:
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,
) {}
}
This is work in progress
not to be merged yet!
Formidable (IncommingForm)
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 oneFormidable
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:"on"
and"true"
to true and the rest to false).Plugins
I still have more to read through.
Parsers
I still have more to read through.
Misc
Formidable.parse
? It could return aPromise<ParsedForm>
, whereParsedForm
is: