[Client][Form] Bug? for validating nested forms

[vicb]

Description of the feature

Context:

• I am using the form framework in a standalone TS project (no flow).
• relates to vaadin/hilla#290

I would like to be able to validate dependent fields in a child form:

• Parent Form:
  • Child Form 1:
    • field: enabled (bool)
    • field: url (string)
  • Child Form 2:
    • field: enabled (bool)
    • field: url (string) ...
  • Child Form N:
    • field: enabled (bool)
    • field: url (string)

What I am trying to achieve is to validate the url in the child form only if this child form is enabled.

My validator looks like:

class TrackerCallbackValidator implements Validator<ChildModel> {
  constructor(public message: string) {}

  validate(value: ChildModel) {
    if (value.enabled && !this.isValid(value.url)) {
      return { property: 'url' };
    }
    return true;
  }
}

The problem with that is that framework expect the path to the property to be absolute, that is validate should return return { property: 'childN.url' };

This is a problem because this validator is not aware of where is it used (i.e. on which of the child).

Do you agree to say that the behavior could be improved here ?

I would see two ways to improve the API here:

1. pass the model as the last parameter of validate(value, binder, +model),
2. Have the framework prepend the parent name to the property for nested forms.

I am not sure if 1) is desirable as passing the model to the validate function could be error prone.

I have implemented 2) and could share my code if you think it's valuable.

/cc @vlukashov @haijian-vaadin

[platosha]

Thanks for reporting the issue. I think, both improvements make sense here, and they are not mutually exclusive but complement each other.

BTW considering that we already support return {property: model} format, not passing a model parameter seems being an unfortunate oversight.

[vicb]

Thanks for reporting the issue

You're welcome. I think this form framework is a little gem !

I locally fixed this by adding a absolutePropertyPath() to Validation.ts:

export async function runValidator<T>(
  model: AbstractModel<T>,
  validator: Validator<T>,
): Promise<ReadonlyArray<ValueError<T>>> {
  const value = getBinderNode(model).value as T;
  // if model is not required and value empty, do not run any validator
  if (!getBinderNode(model).required && !new Required().validate(value)) {
    return [];
  }
  return (async () => (validator.validate as any)(value, getBinderNode(model).binder, model))().then((result) => {
    if (result === false) {
      return [{ property: getBinderNode(model).name, value, validator, message: validator.message }];
    } else if (result === true || (Array.isArray(result) && result.length === 0)) {
      return [];
    } else if (Array.isArray(result)) {
      return result.map((result2) => ({
        message: validator.message,
        ...absolutePropertyPath(model, result2),
        value,
        validator,
      }));
    } else {
      return [{ message: validator.message, ...absolutePropertyPath(model, result), value, validator }];
    }
  });
}

// transforms the "property" field of the result to an absolute path
function absolutePropertyPath<T>(model: AbstractModel<T>, result: ValidationResult): ValidationResult {
  if (typeof result.property === 'string') {
    const path = getBinderNode(model).name;
    if (path.length > 0) {
      result.property = path + '.' + result.property;
    }
  }
  return result;
}

Also if you decide to pass the a model argument to the validator then it would make sense to update the example in the doc.

Thanks !