Argument parsing issue on different functions.

[ozziest]

Story

This issue is related with this PR: #67

The problem is, some rules functions expect parameters one by one, but others expect an array. For example;

// isBetween.ts
export default (value: any, min: any, max: any, extra?: IContext): boolean => {

// isIn.ts
export default (value: any, options: any[] | string): boolean => {

That doesn't make sense. Parsing parameters can't cover both scenarios.

[christoph-kluge]

@ozziest I initially removed the spread on validate.ts and discovered the same thing. Some other tests failed, which lead me to the fact that that not all validation rules follow the same callback structure.

#67 would fix it in 2.x without introducing a breaking change. At least I didn't found a way how I could differentiate between in:1,2 and between:1,2 without adding a hard-coded check for list-like options:

        const isRuleValid = await rule.callback(
          check.value,
-          ...[...rule.params, context]
+          ['in', 'notIn', 'anotherRule'].includes(rule.name) 
+            ? [...rule.params, context] 
+            : { ...[...rule.params, context] }
        );

Sidenote: This wouldn't solve callback-syntax with custom-registered functions, unless you add another option to the register-function registering the rule-name into the ['in', 'notIn', 'anotherRule'] list.

[ozziest]

@christoph-kluge great feedback.

I've implemented a solution like this: https://github.com/axe-api/validator/pull/71/files

Basically, every rule function should have the following parameter structure:

export default (value: any, p1: any, p2: any, extra?: IContext): boolean => {
}

The first parameter should be the value that will be tested. The last parameter might be (doesn't have to be) IContext value if you need to use it.

The other parameters between the value and the extra is the parameter we send by using ,. (For example: in:1,2,3,4).
Doing this allows us to meet all the rule structures, including custom rules.

I'm open to suggestions. ☺

[christoph-kluge]

For v2 either #67 or #71 would be totally fine. At least #71 would cover the context, which was missing in #67 👍

For v3 I'd personally prefer a clean common-callback interface. As an example:

// isBetween.ts
export default (value: any, options: any, extra: IContext): boolean => {
    const [min, max] = ...options
    
    return value >= min && value <= max  // simplified example
}

// isIn.ts
export default (value: any, options: any, extra: IContext): boolean => {
    return options.includes(value) // simplified example
}

Which would change the callback to

const isRuleValid = await rule.callback(
    check.value,
    rule.params,
    context
);

This way each rule could decide on it's own how they interpret the given options. Context would be always given and doesn't require any additional parsing, because it's anyway the last parameter.

[christoph-kluge]

Another idea could be simple rule-helper function for v2? Giving custom rules an easy way to utilize it?

// isIn.tsx
export default (value: any, ...args: any[]): boolean => {
    const {options, context} = parse(args)

    return options.includes(value)
}

What do you think?

[ozziest]

@christoph-kluge , both ideas are great. Thanks for the contribution. ☺

I've checked all the current rules: https://validator.axe-api.com/rules.html

Usually, there are only one or two parameters in the rules. The exceptions are in and notIn. I agree with you about providing a better function structure. The following example comes my mind;

export default (value: any, params: any[], context: IContext): boolean => {
  // check
}

It's similar to your suggestions. ☺ We can push every parameter we can parse to the params. However, the solutions we discussed will require major version changes, and I'm not eager to release major changes frequently. But I'll tag this ticket to remember your suggestion for the major changes. ☺