Promise / async/await support

[nervgh]

Hi,

Thanks for this library!

I use koa@2 that supports async/await syntax. My use case is:

const forms = require('forms')

let myForm = forms.create({
  username: fields.string({ required: true })
})

// POST request handler
app.use(async function(ctx) {
    let boundedForm = myForm.bind(ctx.request.body)
    let errors = await boundedForm.validate() // <-- I need syntax like this
    if (errors) {
      ctx.render('index', {
        form: boundedForm.toHTML()
      })
    } else {
      ctx.body = 'form is valid'
    }
})

What do you think about support of promises (promisification)?

---

Currently, I forced to use my wrapper around this library. It promisifies validate function:

const forms = require('forms')

let {create} = forms

forms.create = function (fields, options) {
  let form = create(fields, options)
  let {bind} = form

  form.bind = function (data) {
    let boundedForm = bind(data)
    // @see https://github.com/caolan/forms/blob/v1.3.0/lib/forms.js#L45
    let {validate} = boundedForm
    boundedForm.validate = function () {
      let deferred = new Deferred()
      validate(deferred.resolve)
      return deferred.promise
    }
    return boundedForm
  }

  return form
}

class Deferred {
  constructor() {
    this.promise = new Promise(this.onPromise.bind(this))
  }
  onPromise(resolve, reject) {
    this.resolve = resolve
    this.reject = reject
  }
  resolve() {}
  reject() {}
}

module.exports = forms

[ljharb]

Your wrapper isn't correct (it ignores error cases), and I wouldn't bother with Deferred, just:

boundedForm.validate = function () {
  return new Promise((resolve, reject) => {
    validate((err, result) => {
      if (err) { reject(err); } else { resolve(result); }
    });
  });
};

This library works down to node 0.4 - using Promise would involve either:

1. a breaking change upping the compat to 0.12 and higher
2. inconsistent behavior on node < 0.12, and >= 0.12
3. including a Promise shim

None of those seem particularly desirable just to avoid a few lines of code :-/

[nervgh]

Thank you for your reply!

Your wrapper isn't correct (it ignores error cases)

It resolves errors (It doesn't throw them).

This library works down to node 0.4

I think we could use semver. I mean we can say [email protected] will require node >=6.5 This will give us power of ECMAScript 2015 standard.

None of those seem particularly desirable just to avoid a few lines of code :-/

Oh, yeah :smile_cat:

[ljharb]

Right, it's not correct if forms provides an error, to resolve the promise - it'd need to reject it.

Of course we can use semver; that's what I meant by "breaking change". But, I'd prefer not to exclude the folks that are still using old node versions.

[voxpelli]

I'm 👍 on this, especially as the oldest LTS release at the moment, for about 3 more months, is Node.js 10.x and it supports most modern API:s https://node.green/

I would actually propose making a 2.0.0 release of forms that targets Node.js 12.x and newer and move to:

• async / await/ Promise
• const/ let
• allow usage of arrow functions where suitable

Thoughts @ljharb @caolan?

I made a related PR: https://github.com/caolan/forms/pull/215 Also, I'm offering help in making this move and to help this module move forward.

Maybe we could move this module to a GitHub organization @caolan?

[ljharb]

I'm not a fan of either dropping support or making breaking changes. It should be possible to support combination callback/Promise APIs.

[voxpelli]

@ljharb In a modules public API:s, I generally agree, but in internal API:s one can make a complete swap if one sees a benefit of it. (One benefit would be that one eg. could use linting rules to enforce a specific style consistently)

It’s also a matter of managing complexity I think. Is it best handled by forcing users to upgrade to a new API or by managing two styles side by side? When the migration doesn’t require a lot of work that equation starts to lean in favor of dropping old API:s I think, but it can vary from API to API.

[ljharb]

True, but until a package adds the "exports" field in package.json (which is semver-major), there's no such thing as "internal APIs".

[voxpelli]

Depends on how one judges the gray area of “non-documented, yet possible to use anyway” API:s.

I import files from forms directly in projects of mine, not because I think that’s supported, but because I was prepared to take the risk of it breaking on every single update, and thus having the intention of upstreaming all such uses and enable documented public API:s for those things.

Just because one can use something one wasn’t intended to one doesn’t have to face no consequences for doing so?

[ljharb]

If it's reachable, it's public ¯\_(ツ)_/¯ certainly a maintainer could choose to be capricious and break someone knowingly, but i certainly wouldn't do that.

[voxpelli]

Right, with that stance I would say that from a complexity standpoint it wouldn’t be feasible to make a move towards a promise based API for this module, if one would then still have to also maintain every callback in every function in every file.

I would love an evolution of the current API that fits better into the current shape of Node.js projects. How would one best achieve that would you say @ljharb? A new module then rather than a breaking change here?

[ljharb]

Give me a bit of time to review your current PRs, and also review the codebase to see what might be a viable approach.

[caolan]

@voxpelli regarding the github org request: if @ljharb would like me to move the repository to an organisation, or simply add new collaborators here, I'm happy to do it. I'd rather not have the complexity of a new organisation for a small module myself, but I'll follow @ljharb's lead.