validators proposal

[vasilich6107]

Hi @joanpablo I have a proposal regarding refactoring of validators approach which could possibly solve the #168 and make validators usage more consistent.

My initial idea is to get rid from callbacks and improve the Validator abstract class to handle all in one

• validation function
• key
• message

look here https://github.com/joanpablo/reactive_forms/compare/master...artflutter:validators-proposal?expand=1#diff-18a9b38eff05410c316cb7546fade4248de3a3ada1ed2f0862dbf5d6cfb236e8L8

This will give us an ability to define validation messages at the validator level like
https://github.com/joanpablo/reactive_forms/compare/master...artflutter:validators-proposal?expand=1#diff-c97e7828e7b481d3fa831aceeba02c4c3ffc0bce2493d766907d2d2496c60071R9

And also override them on control level if needed like
https://github.com/joanpablo/reactive_forms/compare/master...artflutter:validators-proposal?expand=1#diff-c97e7828e7b481d3fa831aceeba02c4c3ffc0bce2493d766907d2d2496c60071R14

This approach will also give another level of consistency for the code generator.

I will be able to generate classes for listing error messages to return from validationMessages fn.
something like if you set a RequiredValidator for email fields - the class EmailValidationMessages will generated.

class EmailValidationMessages {
  final String? reqiredMessage;

  EmailValidationMessages({this.reqiredMessage});

  Map<String, String> messages => {
    'required': reqiredMessage,
  }
}

Let's summarise.

After proposed refactoring we will be able to

• specify common error messages at validator level requested in Globally defined validation messages #168
• ability to override the message at control level
• allow generator to generate more helpful code to help definig error messages in predicatable way

[vasilich6107]

@kuhnroyal @jans-y please join the conversation

[kuhnroyal]

Afaik the error messages are currently located in the UI so that i18n can be used. This would be kind of hard to generate so it probably has to be some kind of error message registry that can be looked up. Maybe global and for each form. But the key here is that it requires a BuildContext to use most i18n libraries.
So I am not exactly sure atm. how to approach #168 - but I don't think the validators are the right place.

Otherwise I agree.

[vasilich6107]

@kuhnroyal in default cases like this
https://github.com/joanpablo/reactive_forms/blob/master/example/lib/samples/login_sample.dart#L19
you have an access to context

On generation side this does not look that we will be able to have access to context so adding validators through the annotation will not allow you to specify the message.
This should be compensated by generating EmailValidationMessages which will help to track the required messages.

[kuhnroyal]

I like the strong typing on the validators, having a class for each, also combining the default message with this class is nice.
I am not sure about the EmailValidationMessages class.
I don't think defining messages in the generator input is the way to go.

I commented some thoughts in #168

[vasilich6107]

EmailValidationMessages will be the output of generator.
Generator will take per field validators, extract key from static const field and generate the EmailValidationMessages

With this class it will be easier to override(or specify) messages on per field basis.

[kuhnroyal]

I am using a lot of anonymous validator functions, how would that fit in?

[vasilich6107]

@kuhnroyal there is a possibility that we do not understand each other at some point.
My proposal removes anonymous validators at all and replaces them with classes.

[kuhnroyal]

Ok, thats what I thought. I am fine with that, it is just a big breaking change

[joanpablo]

Hi everyone I have read all the comments. I appreciate a lot all your support and discussion. Here are not only some of my thoughts but also my approach in all my projects in Flutter, and the main reason why Reactive Forms is implemented this way:

1-) Errors Messages are related to the UI, not the models, so messages should not be defined in the models (FormControls or Validators classes). Besides that almost all the i18n implementations (not all) depend on the context object (UI).

2-) You must always use i18n in your projects. Flutter officially recommends using flutter_localizations of course you can use any other but almost all of them follow the same principles. This is the best way to centralize UI messages (besides supporting different languages).

3-) The same way intl package allows you to customize messages with arguments, Reactive Forms allows you to access to the error validation info, like min and actual values of the Validators.min validator, to create more customized UI errors like: "The minimum required value is {min}, but the current value is {actual}". This is completely possible in the current implementation of Reactive Forms, and I have used it in all of my projects.

4-) Regarding the validators as functions or classes, as far as I remember (I will need to refresh all my projects and the origins of Reactive Forms), I think I was looking for a balance between flexibility and Strong Typing System. I do like to use classes, I have OOP background, but also Dart is very flexible and we sometimes could just use the flexibility of the language (just sometimes). Maybe this is something we can rethink, but I guess it is not a real issue right now. Some people are asking me to include a similar class implementation for the AsyncValidators (if you ask me, this async class implementation has more priority right now).

5-) One of the ways to centralize/define the same message for all the Reactive Widgets like "This field is required" is define a default validationMessages at ReactiveForm widget level or MaterialApp level, and not only at ReactiveFormField level.

So instead of (current implementation):

ReactiveForm(
    formGroup: form,
    child: Column(
        children: [
            ReactiveTextField<String>(
                formControlName: 'email',
                validationMessages: (control) => {
                    ValidationMessage.required: context.localizations.requiredFieldMessage
                }
            ),
            ReactiveTextField<String>(
                formControlName: 'password',
                validationMessages: (control) => {
                    ValidationMessage.required: context.localizations.requiredFieldMessage
                }
            )
        ]
    )
),

It could be like (alternative):

ReactiveForm(
    formGroup: form,
    defaultValidationMessages: (control) => {
        ValidationMessage.required: context.localizations.requiredFieldMessage
    },
    child: Column(
        children: [
            ReactiveTextField<String>(
                formControlName: 'email',
            ),
            ReactiveTextField<String>(
                formControlName: 'password',
            )
        ]
    )
),

But there is a caveat here: at ReactiveTextField widget level you have full control and knowledge about the FormControl (data type) argument, but at ReactiveForm (or MaterialApp) widget level you have no way to know about the control and it is just an AbstractControl (I guess it is not a big deal because you only focus on error info, but it is something to take into considerations).

[kuhnroyal]

I absolutely agree regarding i18n, I don't really see the point of having the messages hardcoded in some annotation.

4-) Regarding the validators as functions or classes, as far as I remember (I will need to refresh all my projects and the origins of Reactive Forms), I think I was looking for a balance between flexibility and Strong Typing System. I do like to use classes, I have OOP background, but also Dart is very flexible and we sometimes could just use the flexibility of the language (just sometimes). Maybe this is something we can rethink, but I guess it is not a real issue right now. Some people are asking me to include a similar class implementation for the AsyncValidators (if you ask me, this async class implementation has more priority right now).

I think we could support both, with some good asserts and warnings we can accept existing functions or actual validator classes.

5-) One of the ways to centralize/define the same message for all the Reactive Widgets like "This field is required" is define a default validationMessages at ReactiveForm widget level or MaterialApp level, and not only at ReactiveFormField level.

This is something I am really looking forward to. It would be nice if this uses the "official" Flutter way via LocalizationsDelegate<ReactiveForm> with some sane defaults but allow overriding per field.

[vasilich6107]

So let's summarize:
Error messages in validation classes is not the best approach, this looks fine. May be I'll try to come out with delegate approach.

Regarding the validators refactoring.
Currently we have

• validator class what's the reason for it? Does it serves any purpose?
• separate class which just instantiates the validators
• separate class for validation messages

I think it's a good point to consider for refactoring both validators and asyncValidators

@joanpablo if you have a request for the same class approach for AsyncValidators it's a good time to combine refactoring.
The overall roadmap could have next steps

• introduction classValidators and classAsyncValidtors params
• later after several months - deprecation of validators and asyncValidators params
• later with major update switch validators to class implementation and get rid from verbose classValidators naming.

[joanpablo]

Hi @kuhnroyal,

I would like to hear from you if you have in your code (implemented applications in Flutter) any inline|method|function validator that can not be moved to a separated validator class instance?

I would like you to show me if there is a use case in which moving the validator to a completely separated validator class could be a headache.

Thanks in advance.

[kuhnroyal]

Everything can be moved, it is just a question of time and effort.

[vasilich6107]

Are we going to make some action plan based on this proposal?

[joanpablo]

I started working on a prototype implementation. I first need to understand if this is a possible and viable solution.

[vasilich6107]

Ok
Will close this PR for now