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

Support for Async Validations #25

Closed
andrewhathaway opened this issue Jul 16, 2018 · 8 comments
Closed

Support for Async Validations #25

andrewhathaway opened this issue Jul 16, 2018 · 8 comments

Comments

@andrewhathaway
Copy link

andrewhathaway commented Jul 16, 2018

This looks like a really nice tool, however I don't see any support or documentation on async/promise-based validation.

Is this something that this project plans on supporting or is this outside of the plan? Classic use-case being username validation, for example!

@imbrn
Copy link
Owner

imbrn commented Jul 16, 2018

Thank you @andrewhathaway for your question.

Actually, v8n doesn't have async/promise-based validation yet.

I've been thinking about that, and this is a really nice feature I want to include as soon as possible.

I'm trying to figure out a way to implement that without increasing too much size in the final release file and keep the same simple fluent API.

@sbarfurth
Copy link
Collaborator

@andrewhathaway What kind of syntax would you like for it? Could you provide a concrete example of a validation and the syntax you'd expect?

@andrewhathaway
Copy link
Author

Sure, there are a few approaches that we could come up with, but as of right now...

To define an async-validation rule

v8n.extend({
  usernameAvailable: () =>
    username =>
      // Showing that you must return a Promise, this may be given by
      // another that returns a promise
      new Promise(resolve => {
        // Make an HTTP req, or something
        const request = this.makeSomeRequest(username)
          .then(exists =>
            resolve(exists));
      })
})

This is similar to your current API for defining rules. However it returns a Promise, and could be simplified to the following:

v8n.extend({
  usernameAvailable: () =>
    username =>
      this.makeSomeRequest(username)
        .then(exists => !exists); // Returns `true` if username exists, so flip the value to fail validation
})

Usage

We'd need to be able to distinguish between standard synchronous validations, and the new async validations. Therefore, I propose that testAsync would be added and would run all validation methods asynchronously, returning a Promise.

v8n
  .string()
  .minLength(1)
  .usernameAvailable()
  .testAsync('andrewhathaway')
  .then(valid => {});

*Note: * To keep a similar API to what you have right now, you'd maybe need to use Proxymise internally, to handle chaining of promise calls. Saying that, because validation rules are functions that return functions, you probably could get away without it. :)

@sbarfurth
Copy link
Collaborator

sbarfurth commented Jul 18, 2018

No idea how this would be implemented properly. I've been experimenting, but no solution is ideal. Ideally an async validation should be written like this, where it simply returns the result:

v8n.extend({
  usernameAvailable: () => username =>
      asyncUsernameFunction(username).then(exists => !exists)
});

All the validations should happen parallel, since order doesn't matter. But the issue with that is that we can't evaluate the result of the async function at validation time and therefore not properly reject or resolve it. Since Promise.all() makes sense here, we would need to throw or reject false results, which would require a catch in the validation function the user adds or a passed Promise, which would need to be implemented in every single validation, essentially making the whole library async by default.

Disregarding inverting the result for the moment, this would be a simple Promise wrapping function for async testing. Of course this would rely on validations to use reject and resolve, since Promise.all() won't finish unless all validations resolve and won't fail unless they actually reject.

testAsync(value) {
  const rules = this.chain.map(rule => {
    return new Promise((resolve, reject) => {
      resolve(rule.fn(value, resolve, reject));
    });
  });

  return Promise.all(rules);
},
v8n.extend({
  usernameAvailable: () => (username, resolve, reject) =>
      asyncUsernameFunction(username).then(exists => {
        if (exists) reject(); // Reject if the username is taken
        resolve(); // Otherwise we resolve
      })
});
expect(
  v8n()
    .usernameAvailable()
    .testAsync("random")
).resolves.toBe(undefined); // Given that username is not taken, this would PASS

The above would work, if it was the only validation. Adding any of the others will cause Promise.all() to never finish unless usernameAvailable rejects.

expect(
  v8n()
    .string()
    .usernameAvailable() // Imagine it's not taken
    .testAsync("random")
).resolves.toBe(undefined); // Will never resolve since string() doesn't implement resolve()

Another option might be to simply wrap the rule functions into Promise.all() without an additional wrapper Promise.

testAsync(value) {
  const rules = this.chain.map(rule => {
    return rule.fn(value);
  });

  return Promise.all(rules);
},

This will always resolve, since even the async validations simply return either true or false. Now that means we always receive an array back from Promise.all(). The resolve and reject values don't really matter, since the result is really our whole interest. We would then need to somehow verify false is not in the array.

// The username is not taken, this should resolve
expect(
  v8n()
    .string()
    .usernameAvailable()
    .testAsync("random")
).resolves.toBe(undefined); // FAIL, resolves to [true, true]

// The username is taken, this should reject
expect(
  v8n()
    .string()
    .usernameAvailable()
    .testAsync("andrewhathaway")
).rejects.toBe(undefined); // FAIL, resolves to [true, false]

As I understand we want to resolve when all the validations pass and reject if any fails. Sadly as per the explanation above I can't figure out a way to do that parallel.

@imbrn
Copy link
Owner

imbrn commented Jul 18, 2018

Actually, we do need to keep the validation in sequence, because in a case like:

v8n()
  .email()       // Synchronous test if the email is valid
  .not.exist()   // Asynchronous test if email does not exists in a server
  .asyncTest("this_is_not_an_email")
  .then(valid => {
    // valid == false
  });

it should first test if the value is actually a valid email so that we can avoid sending a request to the server.

For the asyncTest we need to "wrap" no promised-based rules, maybe just using Promise.resolve function. And we should return a Promise that resolves to true or false.

But for the asyncCheck we need to return a Promise which resolves when everything is valid and rejects with the failed rule. So that we keep the API consistent.

@sbarfurth
Copy link
Collaborator

I would argue we should reject to false and resolve to true, since false is basically an exception.

@imbrn
Copy link
Owner

imbrn commented Jul 18, 2018

I'm now working on a solution for this feature, and I'm going to push that to a separated branch later, so we can discuss more the process I'm using to perform the async validations.

@imbrn
Copy link
Owner

imbrn commented Jul 18, 2018

I've made a pull request (#34) for us to discuss and review a solution. The documentation is also not completed yet.

@imbrn imbrn added this to the v1.1.0 milestone Jul 20, 2018
@imbrn imbrn closed this as completed Jul 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants