Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ng-core] Provide validators #76

Closed
A77AY opened this issue May 19, 2022 · 14 comments
Closed

[ng-core] Provide validators #76

A77AY opened this issue May 19, 2022 · 14 comments

Comments

@A77AY
Copy link

A77AY commented May 19, 2022

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.

@A77AY A77AY changed the title Provide validators [ng-core] Provide validators May 19, 2022
@ersimont
Copy link
Member

ersimont commented May 20, 2022

Hey cool! Thank you for the PR. It looks like this tackles one piece of #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
Copy link
Member

@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
Copy link

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
Copy link
Author

A77AY commented May 20, 2022

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
Copy link
Member

@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
Copy link
Author

A77AY commented May 21, 2022

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
Copy link

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
Copy link
Member

ersimont commented May 25, 2022

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
Copy link

It's documented here
Kara also talks about it here

@ersimont
Copy link
Member

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
Copy link
Member

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 added a commit that referenced this issue Jun 5, 2022
… errors with its outer `NgControl`, if one exists

BREAKING CHANGE: Errors from validation outside and inside a `WrappedControlSuperclass` are now synchronized both ways. See the docs for examples to modify or disable this process.

Closes #76
@ersimont
Copy link
Member

ersimont commented Jun 5, 2022

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
Copy link
Member

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

14.0.0 is blocked on angular-eslint/angular-eslint#1004, which I assume will be cleared soon.

@ersimont
Copy link
Member

Published.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants