simontonsoftware / s-libs

A collection of libraries for any of javascript, rxjs, or angular.
MIT License
43 stars 5 forks source link

[ng-core] Provide validators #76

Closed A77AY closed 2 years ago

A77AY commented 2 years ago

I suggest adding internal validation. Sometimes custom controls need to validate something by default, such as mail, card data, and other business entities.

I did a basic PR, but I didn't find any installation information to run and write tests.

ersimont commented 2 years ago

Hey cool! Thank you for the PR. It looks like this tackles one piece of https://github.com/simontonsoftware/s-libs/issues/41.

Looking at the sample code Dmitry wrote for that issue, I see you took different approaches. Or, actually, it looks like Dmitry was concerned with validation errors that are set on the wrapper control and propagates them to the inner control, whereas you go the opposite direction.

Maybe it's time for me to roll up my sleeves and get back to this! (There was an Angular issue that prevented it the last couple times I tried, but I think it has finally been resolved.)

ersimont commented 2 years ago

@DmitryEfimenko I've come back around to thinking about synchronizing things between outer and inner form controls again. 🙂

@KrickRay and @DmitryEfimenko can you think of any examples when users would NOT want validation to be synchronized from outer to inner controls, or inner to outer? I.e. can you think of a reason a user would need to turn synchronization OFF?

Wanted Not wanted
Outer -> Inner Add required to outer, have default messaging for it inside the component (the component may not always be required, so the outer user needs to opt in) ?
Inner -> Outer Add email validation to component, have it turn the outer form invalid (the component should always have email validation, so we don't want the outer user to have to opt in every time) ?
DmitryEfimenko commented 2 years ago

I'm not sure what you mean in the first example (Outer -> Inner) when you say "so the outer user needs to opt in". Opt in to synchronize validation? But I thought that it's syncronized by default

A77AY commented 2 years ago

Outer -> Inner

With external errors, I see problems:

  1. Outer validators can affect the internal logic if I want to do something with inner errors.
  2. Even if you receive outer errors inside, it will only be possible to highlight, but the text of the error cannot be displayed, since there will only be a key.

Inner -> Outer

If the developer decides to do internal validation, then it must be mandatory and cannot be disabled. The library only allows you to add it

If he wants to make it optional, then he has two options:

  1. Add attributes: required, min, max, etc. which can be added
  2. Write a separate validator that can only be added outside
ersimont commented 2 years ago

@DmitryEfimenko By "opt in" I mean the user must be able to choose whether or not the component is required depending on the situation. The user must have the ability to opt in to that validation, rather than having it unconditionally required everywhere. I'm evaluating whether it should be always synchronized, optionally synchronized, or never synchronized in each direction.

@KrickRay I'm not fully following, I'm sorry to say. Do you have example use cases in mind? (Or can you make up plausible ones?)

A77AY commented 2 years ago

For the option I proposed (inner->outer), and so it will be optional for the developer and he will be able to decide. For the outer->inner, I think that it can bring side effects and it's better to make it optional or you need a separate class for full synchronization.

The last update was difficult, the library changed naming, and if more new behavior is added, this will add new difficulties. Better if the new behavior is optional.

Direction Wanted Not wanted
Outer -> Inner Add required to outer, have default messaging for it inside the component (the component may not always be required, so the outer user needs to opt in) Developer uses the "invalidCharacters" key inside and display the error "The form contains an invalid character X" and it is not valid, but developer using his library added a validator with the same key that will check for others, which will affect the behavior of the form
Inner -> Outer Add email validation to component, have it turn the outer form invalid (the component should always have email validation, so we don't want the outer user to have to opt in every time) The field is optional and it doesn't matter to me that the mail is not valid and I don't need it to affect the validation of the form

About examples, I personally need the inner->outer to get validation from nested FormGroups (this is really how I use it).

DmitryEfimenko commented 2 years ago

I remember why I wanted to sync outer to inner. The only use-case I had for it was to style inputs as invalid. I didn't care what exact error the outer control had - just needed the red outline on the inner inputs that you get automatically due to the ng-invalid class applied by Angular to the inputs that have associated invalid control.

I didn't come across a use case where I needed to come up with a solution to custom sync inner to outer errors. What I mean is that providing standard NG_VALIDATORS and implementing the validate method was covering all use cases I had.

ersimont commented 2 years ago

I definitely see the usefulness of synchronization both ways. Thank you for the examples.

I can use NG_VALIDATORS to sync either direction, but I haven't found a good way to make it sync both directions. It may require injecting NgControl as Dmitry did in his solution. Dmitry: I feel like we've had this conversation before but I can't find it - is the ability to inject NgControl a documented feature? I can't find it, which makes me nervous that it could be an implementation detail of NgModel or something, that could break at any time.

DmitryEfimenko commented 2 years ago

It's documented here Kara also talks about it here

ersimont commented 2 years ago

It's documented here Kara also talks about it here

Ah yes, it's coming back to me. You pointed me to the video before. Thank you.

After playing around a while I think I have the right solution figured out: Inject NG__VALIDATORS to synchronize outer->inner, and NgControl for inner->outer. This will allow validators applied outside to work even when there is no NgControl (such as when using s-libs' own nasModel).

Inner->outer only makes sense when there is an outer NgControl (since otherwise there is nothing to synchronize out to).

Does that sound right to you guys?

ersimont commented 2 years ago

Ah - sorry for the spam. I already realized that doesn't handle reactive forms where the user manually adds validators to the outer control. New proposal: sync both directions with NgControl (as Dmitry did in his example), and fall back to NG_VALIDATORS for outer-> inner if that doesn't exist.

That covers all the scenarios I can think of except when something other than NgControl wants to know about inner validation errors. Maybe there is another custom directive the user adds to the component that tries to inject NG_VALIDATORS for some purpose, like adding special classes to the host. That seems like the least likely use case - is that fair?

ersimont commented 2 years ago

Support is coming with version 14. See the commit that Github auto-linked above. Feel free to look it over and offer any early feedback if desired!

ersimont commented 2 years ago

I just released 14.0.0-next.0, where you can experiment with this if desired!

14.0.0 is blocked on https://github.com/angular-eslint/angular-eslint/pull/1004, which I assume will be cleared soon.

ersimont commented 2 years ago

Published.