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

Promise support for form.parse() #685

Closed
lmammino opened this issue Feb 16, 2021 · 32 comments
Closed

Promise support for form.parse() #685

lmammino opened this issue Feb 16, 2021 · 32 comments
Labels
Area: APIs Things related to external and internal APIs Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: In Progress This issue is being worked on, and has someone assigned. Status: Proposal A feature request or another suggestion. Type: Enhancement Most issues will probably be for additions or changes. Expected that this will result in a PR.

Comments

@lmammino
Copy link

lmammino commented Feb 16, 2021

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

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

@lmammino
Copy link
Author

Awesome, thanks for the quick reply!

@GrosSacASac
Copy link
Contributor

If you want to help review this PR #686

@lmammino
Copy link
Author

Of course. Added some comments there. :)

@tunnckoCore
Copy link
Member

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

@GrosSacASac
Copy link
Contributor

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

@pubmikeb
Copy link

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

@GrosSacASac
Copy link
Contributor

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

@GrosSacASac
Copy link
Contributor

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

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

jwerre commented Mar 29, 2022

Still no Promise support?

@GrosSacASac
Copy link
Contributor

Soon ™

@jwerre
Copy link

jwerre commented Mar 30, 2022

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

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

@GrosSacASac haha 🤣 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.

@tunnckoCore tunnckoCore added Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: In Progress This issue is being worked on, and has someone assigned. Status: Proposal A feature request or another suggestion. Type: Enhancement Most issues will probably be for additions or changes. Expected that this will result in a PR. Area: APIs Things related to external and internal APIs and removed feature request labels Apr 2, 2022
@james-bulb
Copy link

Is there any update on this?

@a1um1
Copy link

a1um1 commented Nov 21, 2022

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

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

ds2345 commented Feb 5, 2023

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

tunnckoCore commented Feb 17, 2023

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

@XinwenCheng
Copy link

XinwenCheng commented Mar 25, 2023

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

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?

@GrosSacASac
Copy link
Contributor

Soon ™

@GrosSacASac
Copy link
Contributor

It is not delayed because it is hard, but because I was busy and I try to fix all the test, see the ongoing PR for commit details

@GrosSacASac
Copy link
Contributor

So based on votes I decided to make it both callback and promise as a transition and then later make it promise only (breaking change)

@GrosSacASac
Copy link
Contributor

#940

@nike1v
Copy link

nike1v commented Jun 16, 2023

Awesome job! Thanks.
Also, I should mention, can you update npm so the types for the library will be updated too? TY

@GrosSacASac
Copy link
Contributor

It will be on npm once pr is merged

@GrosSacASac
Copy link
Contributor

Published as [email protected]

@karlhorky
Copy link

karlhorky commented Jun 18, 2023

Oh nice, v3 is now latest on npm 👀 🔥 Thanks for finally publishing v3!!

However... would be good to get some documentation / types!

I've documented this a bit:


Migration guide?

Unfortunately, in upgrading from [email protected] to [email protected] just now, it appears that our code is breaking.

I was looking for a "Migrating from v2 to v3" guide, I only found this Version History file which seems more like (partly outdated) historical information:

Screenshot 2023-06-18 at 18 35 43

But maybe this is not documentation that the Formidable team will provide? 😬


Release notes?

There are also release notes for v3.0.0, which seem to potentially have breaking changes, although none of them are explicitly called out as such:

3.0.0


Broken types?

Maybe the DefinitelyTyped types at @types/formidable are also outdated now, because of the data structure changes described in the 3.0.0 release notes.

cc @martin-badin @gboer @peterblazejewicz as "code owners" of @types/formidable


Soo... looks like it's time for a bit of trial and error to see how to get this working... 😬

@karlhorky
Copy link

karlhorky commented Jun 18, 2023

The problems in our project was associated with this change from the v3 changelog:

  • files and fields values are always arrays

Our code expected a single file as an object instead of an array with a single item inside.

It appears @types/formidable is already compatible with this change.

@karlhorky
Copy link

karlhorky commented Jun 19, 2023

I did a PR for @types/formidable to add a promise overload for form.parse():

@peterblazejewicz
Copy link

Maybe the DefinitelyTyped types at @types/formidable are also outdated now, because of the data structure changes described in the 3.0.0 release notes.

this could be, I'm not sure if tests on DT for v2 covers code patterns that are changed in v3 (including changed imports, module type direct support, etc)

typescript-bot pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Jun 30, 2023
…karlhorky

* Change version number

* Add overload for promise version of form.parse

PR: node-formidable/formidable#940
Issue: node-formidable/formidable#685

* Add test

* Fix version

* Remove patch version
Desplandis pushed a commit to Desplandis/DefinitelyTyped that referenced this issue Jul 3, 2023
…ise overload by @karlhorky

* Change version number

* Add overload for promise version of form.parse

PR: node-formidable/formidable#940
Issue: node-formidable/formidable#685

* Add test

* Fix version

* Remove patch version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: APIs Things related to external and internal APIs Priority: High After critical issues are fixed, these should be dealt with before any further issues. Status: In Progress This issue is being worked on, and has someone assigned. Status: Proposal A feature request or another suggestion. Type: Enhancement Most issues will probably be for additions or changes. Expected that this will result in a PR.
Projects
None yet
Development

No branches or pull requests