Attaching 'data' listeners immediately

[mxschmitt]

Support plan

• Which support plan is this issue covered by? (Community, Sponsor, Enterprise): Community
• Currently blocking your project/work? (yes/no): yes
• Affecting a production system? (yes/no): OSS - we can't update to formidable v3

Context

• Node.js version: 22
• Release Line of Formidable (Legacy, Current, Next): Current
• Formidable exact version: v3
• Environment (node, browser, native, OS): Node.js/macOS
• Used with (popular names of modules):

What are you trying to achieve or the steps to reproduce?

In formidable v2/v3 the parsing is async and the 'data' handlers are attached not directly. This causes an issue that if you have multiple async things happening after the request was started other consumers might consume the buffered 'data' chunks. Then formidable is not able to parse the request anymore since it receives only a part of it.

An easy fix for that would be to attach the handlers earlier - at the beginning when calling parse.

Currently they are attached after writeHeaders: https://github.com/node-formidable/formidable/blob/d0fbec13edc8add54a1afb9ce1a8d3db803f8d47/src/Formidable.js#L233

See https://github.com/microsoft/playwright/issues/35823#issuecomment-2847667026 for more information. We would appreciate if this could considered since it breaks our use-case and potentially also other customers with async/await hops.

What was the result you got?

When there is an async hop which is consuming 'data' as well, formidable is not able to parse the request body.

What result did you expect?

It works like in formidable v1 - there the parsing was sync.

[tunnckoCore]

It's actually happening below that line.

I'll check the ticket in your repo too.

Anyway, please try the next dist-tag, a lot of the stuff around streaming is tricky and especially with events it sucks, so that's cleaned up. It uses async iterators and for of.

Plesse try the v4 (the next dist-tag, it's in monorepo branch), i don't want to bother anymore with that shitty useless layer (a Formidable EventEmitter class) that's sitting on top of the actual parser.

The parsing wasn't async, nor sync. And isn't now. The buggy layer on top of the core parser is what's causing all the issues and weirdnesses. The parse method just got async in some version. In fact it always was async, just with callbacks back in the days.

It's not the v3 that's the problem, but something else, because we were always async, and we were always reading data events from req at that same place.

[tunnckoCore]

responded there too 🙌

@mxschmitt

[mxschmitt]

It looks like with the next tag the parseMultipartRequest function only accepts a Request object but no IncomingMessage from the node:http std library anymore - what do you recommend for such use-cases?

[tunnckoCore]

@mxschmitt yeah, types are messy, just assert it as http.IncomingMessage.

We are not touching anything special except req.headers (and body of course) so it's basically working. In fact, that's the beauty of new things, they all just work across run-times with great higher level APIs as for await of.

I'd have separate export path for node probably with fixed types.

[mxschmitt]

Looks like it throws then when trying to pass a http.IncomingMessage into parseMultipartRequest:

diff --git a/package.json b/package.json
index 206903764..a8060c9da 100644
--- a/package.json
+++ b/package.json
@@ -57,7 +57,6 @@
     "@eslint/js": "^9.19.0",
     "@stylistic/eslint-plugin": "^3.0.1",
     "@types/codemirror": "^5.60.7",
-    "@types/formidable": "^2.0.4",
     "@types/immutable": "^3.8.7",
     "@types/node": "^18.19.68",
     "@types/react": "^18.0.12",
@@ -84,7 +83,7 @@
     "eslint-plugin-notice": "^1.0.0",
     "eslint-plugin-react": "^7.37.4",
     "eslint-plugin-react-hooks": "^5.1.0",
-    "formidable": "^2.1.1",
+    "formidable": "^4.0.0-rc.6",
     "immutable": "^4.3.7",
     "license-checker": "^25.0.1",
     "mime": "^3.0.0",
diff --git a/tests/page/page-filechooser.spec.ts b/tests/page/page-filechooser.spec.ts
index ba4534547..d966bdf94 100644
--- a/tests/page/page-filechooser.spec.ts
+++ b/tests/page/page-filechooser.spec.ts
@@ -19,7 +19,7 @@ import { test, expect } from './pageTest';
 import { attachFrame } from '../config/utils';
 
 import fs from 'fs';
-import formidable from 'formidable';
+import { parseMultipartRequest } from 'formidable';
 
 test('should upload multiple large files', async ({ page, server, isAndroid, isWebView2, mode }, testInfo) => {
   test.skip(isAndroid);
@@ -247,11 +247,12 @@ test('should not trim big uploaded files', async ({ page, server }) => {
 
   let files: Record<string, formidable.File>;
   server.setRoute('/upload', async (req, res) => {
-    const form = new formidable.IncomingForm();
-    form.parse(req, function(err, fields, f) {
-      files = f as Record<string, formidable.File>;
-      res.end();
-    });
+    await parseMultipartRequest(req, parts => console.log(parts));
+    // const form = new formidable.IncomingForm();
+    // form.parse(req, function(err, fields, f) {
+    //   files = f as Record<string, formidable.File>;
+    //   res.end();
+    // });
   });
   await page.goto(server.EMPTY_PAGE);
 
diff --git a/tests/tsconfig.json b/tests/tsconfig.json
index a921b85a0..8da4f9f24 100644
--- a/tests/tsconfig.json
+++ b/tests/tsconfig.json
@@ -3,7 +3,7 @@
         "allowJs": true,
         "checkJs": false,
         "noEmit": true,
-        "moduleResolution": "node",
+        "moduleResolution": "bundler",
         "target": "ESNext",
         "strictNullChecks": false,
         "strictBindCallApply": true,

with:

    TypeError: request.headers.get is not a function

      248 |   let files: Record<string, formidable.File>;
      249 |   server.setRoute('/upload', async (req, res) => {
    > 250 |     await parseMultipartRequest(req, parts => console.log(parts));

[tunnckoCore]

duh.. weird nodejs 😁🤦‍♂️ so it still doesn't make req.headers a fricking Headers?

You can manually patch that temporarily.

something like

parseMultipartRequest({
  body: req.body,
  headers: new Headers(req.headers)
})