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

[Form lib] Fix regression on field not being validated after reset to its default value. #76379

Merged
merged 2 commits into from
Sep 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,93 @@ describe('<UseField />', () => {
});
});

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<any>();

useEffect(() => {
onForm(form);
}, [onForm, form]);

return (
<Form form={form}>
<UseField path="name" config={fieldConfig} data-test-subj="myField" />
</Form>
);
};
return TestComp;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: TestComp variable is not necessarily needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but sometimes adding the var makes it a little bit clearer when scanning the file than directly returning a function. I guess it depends on my level of fatigue 😊
In this case, I clearly read what is the component without any extra parsing.

};

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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,12 @@ export const useField = <T>(
const [isChangingValue, setIsChangingValue] = useState(false);
const [isValidated, setIsValidated] = useState(false);

const isMounted = useRef<boolean>(false);
const validateCounter = useRef(0);
const changeCounter = useRef(0);
const hasBeenReset = useRef<boolean>(false);
const inflightValidation = useRef<Promise<any> | null>(null);
const debounceTimeout = useRef<NodeJS.Timeout | null>(null);
const isMounted = useRef<boolean>(false);

// -- HELPERS
// ----------------------------------
Expand Down Expand Up @@ -142,11 +143,7 @@ export const useField = <T>(
__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.
if (value !== initialValue && value !== defaultValue) {
await __validateFields(fieldsToValidateOnChange ?? [path]);
}
await __validateFields(fieldsToValidateOnChange ?? [path]);

if (isMounted.current === false) {
return;
Expand All @@ -172,8 +169,6 @@ export const useField = <T>(
}, [
path,
value,
defaultValue,
initialValue,
valueChangeListener,
errorDisplayDelay,
fieldsToValidateOnChange,
Expand Down Expand Up @@ -468,6 +463,7 @@ export const useField = <T>(
setErrors([]);

if (resetValue) {
hasBeenReset.current = true;
const newValue = deserializeValue(updatedDefaultValue ?? defaultValue);
setValue(newValue);
return newValue;
Expand Down Expand Up @@ -539,6 +535,13 @@ export const useField = <T>(
}, [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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 (
<Form form={form}>
<UseField path="username" config={fieldConfig} data-test-subj="myField" />
</Form>
);
};

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".
});
});
});