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

Verify - dependent rules #1628

Closed
davestewart opened this issue Oct 3, 2018 · 9 comments · Fixed by #1634
Closed

Verify - dependent rules #1628

davestewart opened this issue Oct 3, 2018 · 9 comments · Fixed by #1634
Labels
🗯 discussion Open for debate, democracy! ✨ enhancement a "nice to have" feature 🛳 In PR This issue is being addressed in a PR

Comments

@davestewart
Copy link
Collaborator

davestewart commented Oct 3, 2018

The docs state that validator.verify() cannot use dependent rules without v-validate:

Note that Target Dependant Rules won't work as they require a target field being tracked which cannot be done without using the v-validate directive.

However, I don't understand why this is so.

Surely as an independent function, the additional values / rules could be passed in?

validator.verify(value, rule, values, rules)

In my case, I will have all these, so it should be simple to derive a response?

@logaretm
Copy link
Owner

logaretm commented Oct 4, 2018

Since the verify is more of a programmatic API and thus have no access to the refs or the fields data in general. Also I couldn't think of a good API to pass the dependents data, what you suggested might work fine but consider something like this: required|confirmed:a|after:b.

Until I can come up with a good way to use it I'm afraid it will be left out, also drafting this API is critical for #1625 to support these types of rules.

I was thinking about something like this:

validator.verify(value, rule, {
  // validation options?
  deps: {
    confirmed: 'otherValue',
    after: 'afterValue'
  }
});

A little verbose yes, but very flexible and will hardly change in the future. What do you think?

@logaretm logaretm added ✨ enhancement a "nice to have" feature 🗯 discussion Open for debate, democracy! labels Oct 4, 2018
@davestewart
Copy link
Collaborator Author

Hey Abdel,

I'm curious as to why it needs access to refs anyway; are we not just passing in data?

Maybe I am using this incorrectly anyway?

To test, I used a basic validator:

import VeeValidate from 'vee-validate'
const Validator = VeeValidate.Validator

const rules = {
  password1: 'required|alpha|confirmed:password2',
  password2: 'required|alpha|confirmed:password1',
}

const values = {
  password1: 'hello',
  password2: 'hello'
}

const validator = new Validator(rules, {})

validator
  .validateAll(values)
  .then(result => {
    console.log(result ? 'pass' : 'fail')
    console.log(validator.errors)
  })
fail at result ? 'pass' : 'fail' quokka.es6:19:4

ErrorBag { [Iterator] 
  vmId: null,
  items: 
   [ { id: '1',
       vmId: null,
       field: 'password1',
       msg: 'The password1 confirmation does not match.',
       rule: 'confirmed',
       scope: null,
       regenerate: [λ: regenerate] },
     { id: '2',
       vmId: null,
       field: 'password2',
       msg: 'The password2 confirmation does not match.',
       rule: 'confirmed',
       scope: null,
       regenerate: [λ: regenerate] } ] }
	at validator.errors quokka.es6:20:4

Should this not validate?

@davestewart
Copy link
Collaborator Author

davestewart commented Oct 4, 2018

Regarding your thoughts on verify, so something like this:

/**
 * Verify a value
 *
 * @param {string}  value     The value to validate
 * @param {string}  rule      The rule to process the value with
 * @param {object}  options   A hash of options that the validator may need to cross-check values
 */
function verify (value, rule, options) {
  // verify
}

const rules = {
  username:  'required|alpha|min:5',
  password1: 'required:password|matches:password2',
  password2: 'required:password|matches:password1'
}

const values = {
  username:  'dave',
  password1: 'letmein',
  password2: 'letmeinn'
}

const options = {
  values,                 // other form values, to look up if needed
  field: 'password1'      // name of validated field, needed to (optionally) replace {field} in returned error message
}

verify(values.password1, rules.password1, options)

@logaretm
Copy link
Owner

logaretm commented Oct 5, 2018

What I meant by the ref argument, that while the rules conveniently only care about the passed values rather than how they are they resolved. The implementation only has one way of dealing with those fields i.e: fetching them using refs. Cross-checking values with programatic API wasn't implemented.

Your second argument while is expected of validateAll, it does not do any cross-checking for other values, I think this should be corrected in future versions.

I like the API proposed by you here, I might implement it like you suggested.

@davestewart
Copy link
Collaborator Author

davestewart commented Oct 5, 2018

Well, either way!

Let me know how you go. I released Vee Element this morning:

If we can get dependent rules validating here, I will be a happy man!

Unless you have any better suggestions, like implementing a new Validator instance?

@logaretm
Copy link
Owner

logaretm commented Oct 5, 2018

Congrats on the plugin release 🎉 Will be sure to mention it in the docs and here in the README.

Currently my work on #1625 has opened my eyes to simpler approach to how validation should be done. Functional approach to fields should reduce the footprint of vee-validate in the future, I'm planning a re-write once I'm done with the Army service in few months, the re-write have a few goals:

  • Modular Features (Optional ErrorBag, Flags, Classes, Messages) Which means the core lib would be only validating via the verify API since it is the most flexible among the validation methods, this should bring the size down considerably since it is the downside of vee-validate.
  • TypeScript? Haven't decided yet, but flow isn't really proofing the code and is missing most of TS features, also being optional encourages my laziness to not actively proof it.
  • Review some features like Scopes and if they should be removed or not.

This might warrant a major release 3.x but this list isn't definitive. It is the few ideas and regrets about the project, feel free to suggest more.

@logaretm logaretm added the 🛳 In PR This issue is being addressed in a PR label Oct 5, 2018
@davestewart
Copy link
Collaborator Author

Ahhh, I wondered what Army meant in your Twitter bio!

Are you using TS yet? We are at work now, and it's pretty great! But also infuriating at times - though normally because you don't know it well enough. Though we suspect sometimes because there's still improvements to be made 😝 .

Yeah, nothing wrong with a 3.0 release if you have to.

So not really sure where to leave this thread... close it? Sounds like the current API has its limitations.

Will you release a minor update with options as a final arg?

@logaretm
Copy link
Owner

logaretm commented Oct 5, 2018

The options will be released with the next release around this week since it doesn't break anything. I'm still learning TS at the moment, once I'm comfortable with it I will decide whether I would it use it or not.

@davestewart
Copy link
Collaborator Author

The options will be released with the next release around this week

Woohoo!

I'm still learning TS at the moment

Feel free to hit me up on Twitter / email - if I can help, I will

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗯 discussion Open for debate, democracy! ✨ enhancement a "nice to have" feature 🛳 In PR This issue is being addressed in a PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants