Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a validation system for input components with a base validator architecture. It establishes the foundation for validating user inputs with support for internationalization through translators.
- Implements a BaseValidator abstract class that serves as the foundation for all validators
- Creates a ValidatorManager to orchestrate multiple validators and collect validation results
- Provides a React hook for easy integration of the validation system in components
- Includes an example IsEmptyStringValidator for string validation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| BaseValidator.ts | Abstract base class defining the validator interface with translator support |
| ValidatorManager.ts | Manager class that handles multiple validators and aggregates validation results |
| IsEmptyStringValidator.ts | Concrete validator implementation for checking empty strings |
| useValidatorManager.ts | React hook for creating and managing validator instances |
| export default abstract class BaseValidator { | ||
| protected _translator!: TranslatorType; | ||
|
|
||
| setTranslator(translator: TranslatorType) { |
There was a problem hiding this comment.
Using the definite assignment assertion (!) on _translator without initialization could lead to runtime errors if setTranslator() is not called before accessing the translator. Consider making the translator a required constructor parameter or providing a default value.
| setTranslator(translator: TranslatorType) { | |
| protected _translator: TranslatorType; | |
| constructor(translator: TranslatorType) { |
| this._validators = this._validators.filter((savedValidator) => savedValidator !== validator); | ||
| } | ||
|
|
||
| validate(value: unknown) { |
There was a problem hiding this comment.
The validate method lacks a return type annotation. Adding ': { isValid: boolean; messages: string[] }' would improve type safety and API clarity.
| validate(value: unknown) { | |
| validate(value: unknown): { isValid: boolean; messages: string[] } { |
| @@ -0,0 +1,14 @@ | |||
| import BaseValidator from '../../validators/BaseValidator'; | |||
|
|
|||
| export interface ValidateReturnType { | |||
There was a problem hiding this comment.
I think we should avoid naming return types as ...ReturnType because this decreases flexibility. E.g. it may feel strange later to create something like:
const alwaysTrueValidation: ValidateReturnType = {...}
I think we should name it e.g. ValidationResult:
| export interface ValidateReturnType { | |
| export interface ValidationResult { |
| .filter((validator: BaseValidator<T>) => !validator.validate(value)) | ||
| .map((validator: BaseValidator<T>) => validator.getErrorMessage()); |
There was a problem hiding this comment.
I guess this is somewhat fine, but it feels wrong to call filter first and then map to collect error messages :D
| .filter((validator: BaseValidator<T>) => !validator.validate(value)) | |
| .map((validator: BaseValidator<T>) => validator.getErrorMessage()); | |
| .reduce((errors , validator) => { | |
| const isValid = validator.validate(value); | |
| if (isValid) { | |
| continue; | |
| } | |
| return [...errors, validator.getErrorMessage()]; | |
| }, []) |
this is longer but faster to read. :)
Description:
For QA:
Documentation: