Promise support for form.parse()

lmammino commented 3 years ago

I am thinking that it could be nice to make form.parse() promisified: if no callback is provided, it will return a promise.

This could allow us to rewrite the following code:

form.parse(req, (err, fields, files) => {
  if (err) {
    // ... handle error
    return
  }

  // do something with fields and files
})

Into something like this:

try {
  const [fields, files] = await form.parse(req)
  // ... do something with fields and files
} catch (e) {
  // ... handle error
}

Is this something that has been considered already?

I had a quick look at the code and it seems that parse currently returns this (i suppose to allow method chaining). This seems to be a potential blocker for this feature (as it will require a breaking change), but as an alternative, it could be possible to create a new method called parsePromise or something similar, which can act as a wrapper for the original parse method...

The main advantages I see in supporting this syntax are the following:

Unified error handling (one catch to rule them all)
Integrates well with async handlers where there might be other async logic before or after the form parsing
Fewer callbacks nesting

Thoughts?

If this idea resonates with the maintainers and the community, I'd be happy to find some time to submit a PR...

GrosSacASac commented 3 years ago

Wait for v2 to be finalized. We will try to make no breaking change for v2 but for v3 this could be good.

lmammino commented 3 years ago

Awesome, thanks for the quick reply!

GrosSacASac commented 3 years ago

If you want to help review this PR https://github.com/node-formidable/formidable/pull/686

lmammino commented 3 years ago

Of course. Added some comments there. :)

tunnckoCore commented 3 years ago

Yep, it was considered. Maybe we can introduce it in v2 as parseAsync, later in v 3 just merge them.

GrosSacASac commented 3 years ago

@lmammino you can make a PR against the v3.x branch

pubmikeb commented 3 years ago

Are async/await already available in the upcoming v.3.0?

GrosSacASac commented 3 years ago

No the issue is still open We are waiting for someone to make a PR

GrosSacASac commented 3 years ago

Should form.parse()

awlays return a promise (vote with 👍 )
or be callback based if a function is passed to it and else return a promise (vote with ❤️ )
other 😕 (explain in comment)

tunnckoCore commented 3 years ago

@GrosSacASac

or be callback based if a function is passed to it and else return a promise

for v2 and above. Then we should consider the Streaming things and APIs, but lets that be v4 haha.

jwerre commented 2 years ago

Still no Promise support?

GrosSacASac commented 2 years ago

Soon ™

jwerre commented 2 years ago

Just a thought... instead of supporting both callbacks and promises in the same function you could mimic the way fs does it. Something like:

// For Callback
import { formidable } from 'formidable'; 
const formidable = require('formidable');

// For Promise
import { formidable } from 'formidable/promise';
const formidable = require('formidable').promises;

tunnckoCore commented 2 years ago

@jwerre yep, i'm leaning more towards that, or just a separate exports.

@GrosSacASac haha :rofl: on v3 or for v4? I think I said we will drop v2 (as main) around march or april? Uh, let me see where that thing was haha.

james-bulb commented 1 year ago

Is there any update on this?

a1um1 commented 1 year ago

How wrapping function in promise would take a year?

const [fields, files] = (await new Promise(async (r, re) => {
    await form.parse(req, async (err, fields, files) => {
      if (err) return re(err)
      return [fields, files]
    })
  })) as [formidable.Fields, formidable.Files]

but please do coverage test on a code above

tunnckoCore commented 1 year ago

It's not hard. It's just that there are several other things and at least 2 parallel versions used.

Plus, it's that hard to just use util.promisify, or whatever it was called, on form.parse. It's a standard callback function anyway.

ds2345 commented 1 year ago

here is a "one line" solution for creating the promise

const parseForm = async (req) => new Promise((resolve, reject) => new formidable.IncomingForm().parse(req, (err, fields, files) => err ? reject(err) : resolve([fields, files])))

usage:

const [fields, files] = await parseForm(req)

tunnckoCore commented 1 year ago

@ds2345 right. The beauty of callback APIs, you can easily turn them into promises.. haha.

XinwenCheng commented 1 year ago

Hi @ds2345 , I believe your code works, but it seems my code never enters into callback (err, fields, files) => err ? reject(err) : resolve([fields, files])), no error or exception either, the last log output is START form.parse(). The file is uploaded by axios.postForm(), it's just an 82bytes markdown file. Did I miss anything to make this code work? Thanks in advance!

BTW, the version of formidable I currently use is 2.1.1

async #parseForm(request) {
  return new Promise((resolve, reject) => {
    // const form = formidable({ multiples: true, api: { bodyParser: false } });
    console.log('START form.parse()');
    new formidable.IncomingForm().parse(request, (error, fields, files) => {
      console.log(
        'form.parse() error:',
        error,
        ', fields:',
        fields,
        ', files:',
        files
      );

      error ? reject(error) : resolve([fields, files]);
    });
  });
}

async handle(request) {
  console.time('TaskAttachmentSaveHandler.handle() parseForm');

  const [fields, files] = await this.#parseForm(request);

  console.timeEnd('TaskAttachmentSaveHandler.handle() parseForm');
}

machineghost commented 1 year ago

It's very disappointing to see that a library in 2023 isn't capable of using promises.

I would happily submit a PR, as this fix would be trivial to make ... but from the comments here it sounds like the maintainers are against promises?