-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat(lib): formGroup validator oneOf
#94
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,8 +21,16 @@ import { | |
isNullOrUndefined, | ||
ControlsType, | ||
ArrayPropertyKey, | ||
OneOfValidatorRequiresMoreThanOneFieldError, | ||
OneOfValidatorUnknownFieldError, | ||
} from './ngx-sub-form-utils'; | ||
import { FormGroupOptions, NgxFormWithArrayControls, OnFormUpdate, TypedFormGroup } from './ngx-sub-form.types'; | ||
import { | ||
FormGroupOptions, | ||
NgxFormWithArrayControls, | ||
OnFormUpdate, | ||
TypedFormGroup, | ||
TypedValidatorFn, | ||
} from './ngx-sub-form.types'; | ||
|
||
type MapControlFunction<FormInterface, MapValue> = (ctrl: AbstractControl, key: keyof FormInterface) => MapValue; | ||
type FilterControlFunction<FormInterface> = (ctrl: AbstractControl, key: keyof FormInterface) => boolean; | ||
|
@@ -66,6 +74,63 @@ export abstract class NgxSubFormComponent<ControlInterface, FormInterface = Cont | |
|
||
private controlKeys: (keyof FormInterface)[] = []; | ||
|
||
// instead of having the validators defined in the utils file | ||
// we define them here to have access to the form types | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that really just means we should extract the types to their own file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The point was to get access to the form types without having to specify them when calling the validator but I hear your point... (CF next answer) |
||
|
||
// `ngxSubFormValidators` should be used at a group level validation | ||
// with the `getFormGroupControlOptions` hook | ||
protected ngxSubFormValidators = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I understand this pattern is following the angular pattern of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I started exactly how you described it. Although, I do have to admit that it feels wrong to have validators defined within the class and require the class to be instantiated before being able to use them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making another pass on this one, do we really care about tree shaking here? I wouldn't expect ngx-sub-form to embed plenty of validators (if so they should really be extracted into a separated package IMO). Plus, I'm not sure that using a namespace is better for tree shaking 🤔 but yes could have functions where we need to pass the type to. |
||
oneOf(keysArray: (keyof FormInterface)[][]): TypedValidatorFn<FormInterface> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this an array of arrays? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because one form component may have multiple properties that are polymorphic. Example:
In the case we want to handle them without breaking down into sub components we can do:
|
||
if (!keysArray || !keysArray.length || keysArray.some(keys => !keys || keys.length < 2)) { | ||
throw new OneOfValidatorRequiresMoreThanOneFieldError(); | ||
} | ||
|
||
return (formGroup: TypedFormGroup<FormInterface>) => { | ||
const oneOfErrors: (keyof FormInterface)[][] = keysArray.reduce( | ||
(acc, keys) => { | ||
if (!keys.length) { | ||
return acc; | ||
} | ||
|
||
let nbNotNull = 0; | ||
let cpt = 0; | ||
|
||
while (cpt < keys.length && nbNotNull < 2) { | ||
const key: keyof FormInterface = keys[cpt]; | ||
|
||
const control: AbstractControl | null = formGroup.get(key as string); | ||
|
||
if (!control) { | ||
throw new OneOfValidatorUnknownFieldError(key as string); | ||
} | ||
|
||
if (!isNullOrUndefined(control.value)) { | ||
nbNotNull++; | ||
} | ||
|
||
cpt++; | ||
} | ||
|
||
if (nbNotNull !== 1) { | ||
acc.push(keys); | ||
} | ||
|
||
return acc; | ||
}, | ||
[] as (keyof FormInterface)[][], | ||
); | ||
|
||
if (oneOfErrors.length === 0) { | ||
return null; | ||
} | ||
|
||
return { | ||
oneOf: oneOfErrors, | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be missing some complex requirement here, but this feels like it could be simplified to the following (no idea if this compiles ;) ) const notNullKeys: Array<keyof FormInterface> = keysArray.filter(() => {
const control: AbstractControl | null = formGroup.get(key as string);
if (!control) {
throw new OneOfValidatorUnknownFieldError(key as string);
}
return !isNullOrUndefined(control.value);
});
if (notNullKeys === 1) {
return null;
}
return { oneOf: notNullKeys }; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That code doesn't handle if multiple values are defined. If there's more than 1 value, which one to pick? That should just be an error IMO |
||
}; | ||
}, | ||
}; | ||
|
||
// when developing the lib it's a good idea to set the formGroup type | ||
// to current + `| undefined` to catch a bunch of possible issues | ||
// see @note form-group-undefined | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo