@AndrewKushnir @petebacondarwin once this PR you gives go ahead signal I will raise it similarly for other methods as well

I might have misunderstood the plan, but I thought the idea was to implement a new enabled() method that would be used to decide whether validators in general were enabled or not; and in these minlength and maxlength validators would implement the enabled() check?

No you are correct, let me include it.

hey @AndrewKushnir @petebacondarwin when I converted it to enabled function and typescript began to give me error object possibly null error check screenshot below,

On the hand if I use variable directly I am not getting any error.

Seems typescript is not able to infer the type narrowing, I can even do workaround but it will be overkill.

Quick workaround can be like below

If you have any other suggestion then please let me know.

I wonder if we could structure this with an abstract base class?

@Directive()
abstract class ValidatorBase implements OnChanges, Validator {
  private _validator: ValidatorFn = nullValidator;
  private _onChange?: () => void;

  constructor(private _validatorKey: string) {}

  ngOnChanges(changes: SimpleChanges): void {
    if (this._validatorKey in changes) {
      this._validator = this._enabled() ? this._createValidator() : nullValidator;
      if (this._onChange) this._onChange();
    }
  }

  validate(control: AbstractControl): ValidationErrors|null {
    return this._validator(control);
  }

  registerOnValidatorChange(fn: () => void): void {
    this._onChange = fn;
  }

  protected abstract _createValidator(): ValidatorFn;
  protected abstract _enabled(): boolean;
}

Then you could have:

@Directive({
  selector: '[minlength][formControlName],[minlength][formControl],[minlength][ngModel]', 
  providers: [MIN_LENGTH_VALIDATOR],
  host: {'[attr.minlength]': '_enabled() ? minlength : null'}
})
export class MinLengthValidator implements Validator, OnChanges extends ValidatorBase {
  constructor() {
    super('minlength');
  }

  @Input()  minlength!: string|number|null;

  protected _createValidator(): ValidatorFn {
    // This will only be called if `this.minlength` is non-null
    return minLengthValidator(toInteger(this.minlength!));
  }

  // this may need to be public to use it in the hostbinding.
  protected _enabled(): boolean {
    return this.minlength !== null;
  }
}

@petebacondarwin yes good input but do you think it's fine If I can revised it to.

abstract class ValidatorBase implements OnChanges, Validator {
  private _validator: ValidatorFn = nullValidator;
  private _onChange?: () => void;
  protected prop: number|string|null = null;

  constructor(private _validatorKey: string) {}

  ngOnChanges(changes: SimpleChanges): void {
    if (this._validatorKey in changes) {
      this._validator = this._enabled() ? this._createValidator() : nullValidator;
      if (this._onChange) this._onChange();
    }
  }

  validate(control: AbstractControl): ValidationErrors|null {
    return this._validator(control);
  }

  registerOnValidatorChange(fn: () => void): void {
    this._onChange = fn;
  }

  protected abstract _createValidator(): ValidatorFn;
  
  // this may need to be public to use it in the hostbinding.
  protected _enabled(): boolean {
    return this.prop !== null;
  }
}


@Directive({
  selector: '[minlength][formControlName],[minlength][formControl],[minlength][ngModel]',
  providers: [MIN_LENGTH_VALIDATOR],
  host: {'[attr.minlength]': '_enabled(minlength) ? minlength : null'}
})
export class MinLengthValidator extends ValidatorBase implements Validator, OnChanges {
  constructor() {
    super('minlength');
  }

  @Input()  minlength!: string|number|null;

  prop = this.minlength;

  protected _createValidator(): ValidatorFn {
    // This will only be called if `this.minlength` is non-null
    return minLengthValidator(toInteger(this.minlength!));
  }

I don't really think that the this.prop is very beneficial, especially since it would restrict how we can compute enabled(). Remember that some validators are enabled even if their property is 0.

Thanks for addressing the feedback @iRealNirmal 👍
I've started tests in Google's codebase one last time and if everything goes well, we should be able to add this PR to the merge queue. Will keep this thread updated.

@iRealNirmal FYI the checks in Google's codebase went well, so I'm adding this PR to the merge queue.

Thanks for contributing to Angular!

thanks @AndrewKushnir on staying top for it, I will now will work on this kind of change for min and max validator. Let me know in case anything else.

I will now will work on this kind of change for min and max validator.

Thanks for the help @iRealNirmal 👍 That sounds like a great next step!

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}