Add a promised based handle()

[voxpelli]

Nowadays my feeling is that promise/async/await is pretty much the standard way of doing asynchronous development in node.js, so in my own projects and the projects I did at @Sydsvenskan I use a wrapper that makes .handle() be promised based.

This is that wrapper upstreamed to be part of handle() itself, making the callbacks argument optional and when it's omitted, instead a promise is returned.

[voxpelli]

If this project should stick with es3, then I need to remove the arrow functions here. Maybe could be okay to start moving towards a new major version where things like arrow functions and const / let is allowed? The benefit of sticking to es3 is pretty slim nowadays? (I'm saying that as someone who stuck with es3 for quite some time)

[ljharb]

This looks great :-)

We'll need some tests for this functionality as well.

(There's not enough benefit in using the newer syntax versus the users it excludes, so let's continue using ES3 here)

are there other callback-taking APIs in this library that should get the same treatment? It'd be ideal to update them all at once.

[voxpelli]

are there other callback-taking APIs in this library that should get the same treatment? It'd be ideal to update them all at once.

The only other ones would be the form.validate(callback) and field.validate(callback) pair.

Possibly one would then also allow validator methods to be Promises as they currently instead deliver the result through a (errMessage = undefined) => undefined callback. One could check whether the validator function instead returns a Promise and if it does, then one could use whatever it resolves to instead. (If one did that, then I would also suggest having such a promise reesolve to something like { err: true, message: 'Something'} rather than having the error status be implicitly defined by the existence of an error message)

Maybe fix form.validate(callback) and field.validate(callback) in this PR and do a follow up for possibly accepting promised validators?

[voxpelli]

Right, tests, I'll add, sorry for premature review request. Coming soon!

[voxpelli]

@ljharb There, fixed

In regards to forms.validate(): It uses the async module to do its validation, I'm wondering if it maybe would be sensible to add a polyfill for Promise and port that over to use Promise internally and depending on whether a callback is given, either return that Promise or call the callback with the result of the Promise.

As such I think a new issue / PR for that would make sense? Either one of us create an issue and then I can make a PR

[ljharb]

LGTM! i'll rebase with some changes.

[ljharb]

@voxpelli it's not a good idea to ship a Promise polyfill - especially since most people either won't need one, or will already be shipping their own. We can definitely look into validate in a separate PR, though.

[codecov-io]

Codecov Report

Merging #215 (11200d8) into master (4b99dfa) will increase coverage by 0.32%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   54.86%   55.19%   +0.32%     
==========================================
  Files          12       12              
  Lines        1059     1069      +10     
  Branches      259      261       +2     
==========================================
+ Hits          581      590       +9     
- Misses        478      479       +1     

Impacted Files	Coverage Δ
lib/forms.js	89.65% <90.90%> (+0.04%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b99dfa...11200d8. Read the comment docs.

[voxpelli]

@voxpelli it's not a good idea to ship a Promise polyfill - especially since most people either won't need one, or will already be shipping their own. We can definitely look into validate in a separate PR, though.

I was thinking a ponyfill that would only be used when no global Promise is available, and justify that by the fact that the async module is probably as big as such a ponyfill, but is included for all users today, also those with Promises support.

Should I create an issue for it then?

[ljharb]

Sure, we can discuss it there.