From efa9700adb3955d31f9a6347aa21a238f18ee9ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Tue, 1 Sep 2020 16:00:28 +0200 Subject: [PATCH 1/2] Remove the condition that prevent the field validation to run when value === defaultValue I introduced a regression when I added an "if" condition to prevent validation when the field value === the initial value. This if was added to prevent validating after resetting the field value. This created a regression as now the validation was never run on empty fields. --- .../components/use_field.test.tsx | 87 +++++++++++++++++++ .../forms/hook_form_lib/hooks/use_field.ts | 17 ++-- .../hook_form_lib/hooks/use_form.test.tsx | 60 ++++++++++++- 3 files changed, 157 insertions(+), 7 deletions(-) diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_field.test.tsx b/src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_field.test.tsx index a55b2f0a8fa29..c14471991ccd3 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_field.test.tsx +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/components/use_field.test.tsx @@ -64,6 +64,93 @@ describe('', () => { }); }); + describe('validation', () => { + let formHook: FormHook | null = null; + + beforeEach(() => { + formHook = null; + }); + + const onFormHook = (form: FormHook) => { + formHook = form; + }; + + const getTestComp = (fieldConfig: FieldConfig) => { + const TestComp = ({ onForm }: { onForm: (form: FormHook) => void }) => { + const { form } = useForm(); + + useEffect(() => { + onForm(form); + }, [onForm, form]); + + return ( +
+ + + ); + }; + return TestComp; + }; + + const setup = (fieldConfig: FieldConfig) => { + return registerTestBed(getTestComp(fieldConfig), { + memoryRouter: { wrapComponent: false }, + defaultProps: { onForm: onFormHook }, + })() as TestBed; + }; + + test('should update the form validity whenever the field value changes', async () => { + const fieldConfig: FieldConfig = { + defaultValue: '', // empty string, which is not valid + validations: [ + { + validator: ({ value }) => { + // Validate that string is not empty + if ((value as string).trim() === '') { + return { message: 'Error: field is empty.' }; + } + }, + }, + ], + }; + + // Mount our TestComponent + const { + form: { setInputValue }, + } = setup(fieldConfig); + + if (formHook === null) { + throw new Error('FormHook object has not been set.'); + } + + let { isValid } = formHook; + expect(isValid).toBeUndefined(); // Initially the form validity is undefined... + + await act(async () => { + await formHook!.validate(); // ...until we validate the form + }); + + ({ isValid } = formHook); + expect(isValid).toBe(false); + + // Change to a non empty string to pass validation + await act(async () => { + setInputValue('myField', 'changedValue'); + }); + + ({ isValid } = formHook); + expect(isValid).toBe(true); + + // Change back to an empty string to fail validation + await act(async () => { + setInputValue('myField', ''); + }); + + ({ isValid } = formHook); + expect(isValid).toBe(false); + }); + }); + describe('serializer(), deserializer(), formatter()', () => { interface MyForm { name: string; diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts index fa29f900af2ef..a07c82b3d591e 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts @@ -69,11 +69,12 @@ export const useField = ( const [isChangingValue, setIsChangingValue] = useState(false); const [isValidated, setIsValidated] = useState(false); + const isMounted = useRef(false); const validateCounter = useRef(0); const changeCounter = useRef(0); + const hasBeenReset = useRef(false); const inflightValidation = useRef | null>(null); const debounceTimeout = useRef(null); - const isMounted = useRef(false); // -- HELPERS // ---------------------------------- @@ -144,9 +145,7 @@ export const useField = ( // Validate field(s) (that will update form.isValid state) // We only validate if the value is different than the initial or default value // to avoid validating after a form.reset() call. - if (value !== initialValue && value !== defaultValue) { - await __validateFields(fieldsToValidateOnChange ?? [path]); - } + await __validateFields(fieldsToValidateOnChange ?? [path]); if (isMounted.current === false) { return; @@ -172,8 +171,6 @@ export const useField = ( }, [ path, value, - defaultValue, - initialValue, valueChangeListener, errorDisplayDelay, fieldsToValidateOnChange, @@ -468,6 +465,7 @@ export const useField = ( setErrors([]); if (resetValue) { + hasBeenReset.current = true; const newValue = deserializeValue(updatedDefaultValue ?? defaultValue); setValue(newValue); return newValue; @@ -539,6 +537,13 @@ export const useField = ( }, [path, __removeField]); useEffect(() => { + // If the field value has been reset, we don't want to call the "onValueChange()" + // as it will set the "isPristine" state to true or validate the field, which initially we don't want. + if (hasBeenReset.current) { + hasBeenReset.current = false; + return; + } + if (!isMounted.current) { return; } diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.test.tsx b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.test.tsx index 4a880415b6d22..edcd84daf5d2f 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.test.tsx +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.test.tsx @@ -22,7 +22,13 @@ import { act } from 'react-dom/test-utils'; import { registerTestBed, getRandomString, TestBed } from '../shared_imports'; import { Form, UseField } from '../components'; -import { FormSubmitHandler, OnUpdateHandler, FormHook, ValidationFunc } from '../types'; +import { + FormSubmitHandler, + OnUpdateHandler, + FormHook, + ValidationFunc, + FieldConfig, +} from '../types'; import { useForm } from './use_form'; interface MyForm { @@ -441,5 +447,57 @@ describe('useForm() hook', () => { deeply: { nested: { value: '' } }, // Fallback to empty string as no config was provided }); }); + + test('should not validate the fields after resetting its value (form validity should be undefined)', async () => { + const fieldConfig: FieldConfig = { + defaultValue: '', + validations: [ + { + validator: ({ value }) => { + if ((value as string).trim() === '') { + return { message: 'Error: empty string' }; + } + }, + }, + ], + }; + + const TestResetComp = () => { + const { form } = useForm(); + + useEffect(() => { + formHook = form; + }, [form]); + + return ( +
+ + + ); + }; + + const { + form: { setInputValue }, + } = registerTestBed(TestResetComp, { + memoryRouter: { wrapComponent: false }, + })() as TestBed; + + let { isValid } = formHook!; + expect(isValid).toBeUndefined(); + + await act(async () => { + setInputValue('myField', 'changedValue'); + }); + ({ isValid } = formHook!); + expect(isValid).toBe(true); + + await act(async () => { + // When we reset the form, value is back to "", which is invalid for the field + formHook!.reset(); + }); + + ({ isValid } = formHook!); + expect(isValid).toBeUndefined(); // Make sure it is "undefined" and not "false". + }); }); }); From 7a2c0453d2eb0760e42469fa2861a942b60fd050 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Tue, 1 Sep 2020 16:21:29 +0200 Subject: [PATCH 2/2] Remove comments --- .../es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts index a07c82b3d591e..f01c7226ea4ce 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_field.ts @@ -143,8 +143,6 @@ export const useField = ( __updateFormDataAt(path, value); // Validate field(s) (that will update form.isValid state) - // We only validate if the value is different than the initial or default value - // to avoid validating after a form.reset() call. await __validateFields(fieldsToValidateOnChange ?? [path]); if (isMounted.current === false) {