Adds a model-level validation capability to our validation library #49543
Conversation
Adds a model-level validation capability to our validation library.
bl-nero
added
the
no-changelog
| * fewer items in this list than the labels, the corresponding items will be | ||
| * treated as valid. | ||
| */ | ||
| items?: ValidationResult[]; |
There was a problem hiding this comment.
I think if i received this field in some result i would have no idea what it meant. Based on the comment, should this be validItems or ... unchecked items? (based on label number? im confused)
There was a problem hiding this comment.
@avatus How about itemResults?
There was a problem hiding this comment.
what is the context of the result? valid or not valid? checked vs unchecked? thats the part I'm getting lost with (forgive my brain)
There was a problem hiding this comment.
Oh, I see what you mean. The results are "valid or not valid". In other words, these are per-item validation results that enable us to show a validity state and message for each item of the string list separately.
There was a problem hiding this comment.
Ahhhh, that makes sense. with that context, items probably makes most sense. I think maybe even results might be better but, I'll leave it up to you if you want to make any changes
| rule={precomputed( | ||
| validationResult.items?.[i] ?? { valid: true } | ||
| )} |
There was a problem hiding this comment.
Could you help me understand how this is interacting with default rule? The default rule just returns valid: true, why would we need to do extra validation here based on item length? could we just return whatever the default rule returns here instead of valid: true?
There was a problem hiding this comment.
This { valid: true } object is a per-item object, as opposed to the one returned from the default rule, which applies to all items.
| /** | ||
| * @deprecated For temporary Enterprise compatibility only. Use {@link state} | ||
| * instead. | ||
| */ | ||
| valid = true; |
There was a problem hiding this comment.
is this something to just keep the builds from breaking? I'd add a TODO here to ensure it gets updated after, otherwise this could get messy. I wonder if it'd be better to use a getter here to always return the value of state instead of a separate property. like this pseudo-ish code
get valid() {
return this.state.valid;
}There was a problem hiding this comment.
The cleanup PR #49550 removes it, but I'll follow your advice anyway.
There was a problem hiding this comment.
Ah you're right. I think we can ignore this comment then
There was a problem hiding this comment.
Actually I wanted to suggest using a getter as well. Accessing state.valid in the callsites look a bit off, like accessing an internal property of Validator. I believe having getters for valid and validating would provide a nicer API.
But I will leave it up to you.
| * fewer items in this list than the labels, the corresponding items will be | ||
| * treated as valid. | ||
| */ | ||
| items?: ValidationResult[]; |
| /** | ||
| * @deprecated For temporary Enterprise compatibility only. Use {@link state} | ||
| * instead. | ||
| */ | ||
| valid = true; |
There was a problem hiding this comment.
Actually I wanted to suggest using a getter as well. Accessing state.valid in the callsites look a bit off, like accessing an internal property of Validator. I believe having getters for valid and validating would provide a nicer API.
But I will leave it up to you.
This PR adds a new mechanism for validating the form model at the level of a component that owns the form, as opposed to every form control separately. This is required for the further role editor work, which requires errors to be visible on multiple levels of the hierarchy.
For more context (particularly understanding the use of
runRules), see the upcoming PR #49546, which makes use of these utilities.Requires https://github.com/gravitational/teleport.e/pull/5591
Followed up by #49544
Followed up by https://github.com/gravitational/teleport.e/pull/5592 (cleanup)
Contributes to #46612