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 issue where serializer on fields are called on every change #75166

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 @@ -17,9 +17,10 @@
* under the License.
*/
import React, { useEffect } from 'react';
import { act } from 'react-dom/test-utils';

import { registerTestBed } from '../shared_imports';
import { OnUpdateHandler } from '../types';
import { registerTestBed, TestBed } from '../shared_imports';
import { FormHook, OnUpdateHandler, FieldConfig } from '../types';
import { useForm } from '../hooks/use_form';
import { Form } from './form';
import { UseField } from './use_field';
Expand Down Expand Up @@ -62,4 +63,91 @@ describe('<UseField />', () => {
lastName: 'Snow',
});
});

describe('serializer(), deserializer(), formatter()', () => {
interface MyForm {
name: string;
}

const serializer = jest.fn();
const deserializer = jest.fn();
const formatter = jest.fn();

const fieldConfig: FieldConfig = {
defaultValue: '',
serializer,
deserializer,
formatters: [formatter],
};

let formHook: FormHook<MyForm> | null = null;

beforeEach(() => {
formHook = null;
serializer.mockReset().mockImplementation((value) => `${value}-serialized`);
deserializer.mockReset().mockImplementation((value) => `${value}-deserialized`);
formatter.mockReset().mockImplementation((value: string) => value.toUpperCase());
});

const onFormHook = (_form: FormHook<MyForm>) => {
formHook = _form;
};

const TestComp = ({ onForm }: { onForm: (form: FormHook<MyForm>) => void }) => {
const { form } = useForm<MyForm>({ defaultValue: { name: 'John' } });

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

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

test('should call each handler at expected lifecycle', async () => {
const setup = registerTestBed(TestComp, {
memoryRouter: { wrapComponent: false },
defaultProps: { onForm: onFormHook },
});

const testBed = setup() as TestBed;

if (!formHook) {
throw new Error(
`formHook is not defined. Use the onForm() prop to update the reference to the form hook.`
);
}

const { form } = testBed;

expect(deserializer).toBeCalled();
expect(serializer).not.toBeCalled();
expect(formatter).not.toBeCalled();

let formData = formHook.getFormData({ unflatten: false });
expect(formData.name).toEqual('John-deserialized');

await act(async () => {
form.setInputValue('myField', 'Mike');
});

expect(formatter).toBeCalled(); // Formatters are executed on each value change
expect(serializer).not.toBeCalled(); // Serializer are executed *only** when outputting the form data

formData = formHook.getFormData();
expect(serializer).toBeCalled();
expect(formData.name).toEqual('MIKE-serialized');

// Make sure that when we reset the form values, we don't serialize the fields
serializer.mockReset();

await act(async () => {
formHook!.reset();
});
expect(serializer).not.toBeCalled();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,13 @@ export const useField = <T>(
setIsChangingValue(true);
}

const newValue = serializeOutput(value);

// Notify listener
if (valueChangeListener) {
valueChangeListener(newValue as T);
valueChangeListener(value);
}

// Update the form data observable
__updateFormDataAt(path, newValue);
__updateFormDataAt(path, value);

// Validate field(s) and update form.isValid state
await __validateFields(fieldsToValidateOnChange ?? [path]);
Expand All @@ -153,7 +151,6 @@ export const useField = <T>(
}
}
}, [
serializeOutput,
valueChangeListener,
errorDisplayDelay,
path,
Expand Down Expand Up @@ -442,13 +439,7 @@ export const useField = <T>(

if (resetValue) {
setValue(initialValue);
/**
* Having to call serializeOutput() is a current bug of the lib and will be fixed
* in a future PR. The serializer function should only be called when outputting
* the form data. If we need to continuously format the data while it changes,
* we need to use the field `formatter` config.
*/
return serializeOutput(initialValue);
return initialValue;
}
},
[setValue, serializeOutput, initialValue]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { act } from 'react-dom/test-utils';
import { registerTestBed, getRandomString, TestBed } from '../shared_imports';

import { Form, UseField } from '../components';
import { FormSubmitHandler, OnUpdateHandler } from '../types';
import { FormSubmitHandler, OnUpdateHandler, FormHook, ValidationFunc } from '../types';
import { useForm } from './use_form';

interface MyForm {
Expand Down Expand Up @@ -123,6 +123,71 @@ describe('use_form() hook', () => {

expect(formData).toEqual(expectedData);
});

test('should not build the object if the form is not valid', async () => {
let formHook: FormHook<MyForm> | null = null;

const onFormHook = (_form: FormHook<MyForm>) => {
formHook = _form;
};

const TestComp = ({ onForm }: { onForm: (form: FormHook<MyForm>) => void }) => {
const { form } = useForm<MyForm>({ defaultValue: { username: 'initialValue' } });
const validator: ValidationFunc = ({ value }) => {
if (value === 'wrongValue') {
return { message: 'Error on the field' };
}
};

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

return (
<Form form={form}>
<UseField
path="username"
config={{ validations: [{ validator }] }}
data-test-subj="myField"
/>
</Form>
);
};

const setup = registerTestBed(TestComp, {
defaultProps: { onForm: onFormHook },
memoryRouter: { wrapComponent: false },
});

const {
form: { setInputValue },
} = setup() as TestBed;

if (!formHook) {
throw new Error(
`formHook is not defined. Use the onForm() prop to update the reference to the form hook.`
);
}

let data;
let isValid;

await act(async () => {
({ data, isValid } = await formHook!.submit());
});

expect(isValid).toBe(true);
expect(data).toEqual({ username: 'initialValue' });

setInputValue('myField', 'wrongValue'); // Validation will fail

await act(async () => {
({ data, isValid } = await formHook!.submit());
});

expect(isValid).toBe(false);
expect(data).toEqual({}); // Don't build the object (and call the serializers()) when invalid
});
});

describe('form.subscribe()', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export function useForm<T extends FormData = FormData>(
return Object.entries(fieldsRefs.current).reduce(
(acc, [key, field]) => ({
...acc,
[key]: field.__serializeOutput(),
[key]: field.value,
}),
{} as T
);
Expand Down Expand Up @@ -233,8 +233,7 @@ export function useForm<T extends FormData = FormData>(
fieldsRefs.current[field.path] = field;

if (!{}.hasOwnProperty.call(getFormData$().value, field.path)) {
const fieldValue = field.__serializeOutput();
updateFormDataAt(field.path, fieldValue);
updateFormDataAt(field.path, field.value);
}
},
[getFormData$, updateFormDataAt]
Expand Down Expand Up @@ -301,7 +300,7 @@ export function useForm<T extends FormData = FormData>(
setSubmitting(true);

const isFormValid = await validateAllFields();
const formData = getFormData();
const formData = isFormValid ? getFormData() : ({} as T);

if (onSubmit) {
await onSubmit(formData, isFormValid!);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,17 @@ export const CreateField = React.memo(function CreateFieldComponent({

{/* Field subType (if any) */}
<FormDataProvider pathsToWatch="type">
{({ type }) => (
<SubTypeParameter
key={type}
type={type}
isMultiField={isMultiField ?? false}
isRootLevelField={isRootLevelField}
/>
)}
{({ type }) => {
const [fieldType] = type;
return (
<SubTypeParameter
key={fieldType.value}
type={fieldType.value}
isMultiField={isMultiField ?? false}
isRootLevelField={isRootLevelField}
/>
);
}}
</FormDataProvider>
</EuiFlexGroup>
);
Expand Down Expand Up @@ -188,7 +191,10 @@ export const CreateField = React.memo(function CreateFieldComponent({

<FormDataProvider pathsToWatch={['type', 'subType']}>
{({ type, subType }) => {
const ParametersForm = getParametersFormForType(type, subType);
const ParametersForm = getParametersFormForType(
type?.[0].value,
subType?.[0].value
);

if (!ParametersForm) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,15 +98,15 @@ export const EditField = React.memo(({ form, field, allFields, exitEdit, updateF
<FormDataProvider pathsToWatch={['type', 'subType']}>
{({ type, subType }) => {
const linkDocumentation =
documentationService.getTypeDocLink(subType) ||
documentationService.getTypeDocLink(type);
documentationService.getTypeDocLink(subType?.[0].value) ||
documentationService.getTypeDocLink(type?.[0].value);

if (!linkDocumentation) {
return null;
}

const typeDefinition = TYPE_DEFINITION[type as MainType];
const subTypeDefinition = TYPE_DEFINITION[subType as SubType];
const typeDefinition = TYPE_DEFINITION[type[0].value as MainType];
const subTypeDefinition = TYPE_DEFINITION[subType?.[0].value as SubType];

return (
<EuiFlexItem grow={false}>
Expand Down Expand Up @@ -148,7 +148,7 @@ export const EditField = React.memo(({ form, field, allFields, exitEdit, updateF

<FormDataProvider pathsToWatch={['type', 'subType']}>
{({ type, subType }) => {
const ParametersForm = getParametersFormForType(type, subType);
const ParametersForm = getParametersFormForType(type?.[0].value, subType?.[0].value);

if (!ParametersForm) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,23 +36,26 @@ export const EditFieldHeaderForm = React.memo(

{/* Field subType (if any) */}
<FormDataProvider pathsToWatch="type">
{({ type }) => (
<SubTypeParameter
key={type}
type={type}
defaultValueType={defaultValue.type}
isMultiField={isMultiField}
isRootLevelField={isRootLevelField}
/>
)}
{({ type }) => {
const [fieldType] = type;
return (
<SubTypeParameter
key={fieldType.value}
type={fieldType.value}
defaultValueType={defaultValue.type}
isMultiField={isMultiField}
isRootLevelField={isRootLevelField}
/>
);
}}
</FormDataProvider>
</EuiFlexGroup>

{/* Field description */}
<FieldDescriptionSection isMultiField={isMultiField}>
<FormDataProvider pathsToWatch={['type', 'subType']}>
{({ type, subType }) => {
const typeDefinition = TYPE_DEFINITION[type as MainType];
const typeDefinition = TYPE_DEFINITION[type[0].value as MainType];
const hasSubType = typeDefinition.subTypes !== undefined;

if (hasSubType) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,11 @@ export const reducer = (state: State, action: Action): State => {
return {
...state,
fields: updatedFields,
documentFields: {
...state.documentFields,
// If we removed the last field, show the "Create field" form
status: updatedFields.rootLevelFields.length === 0 ? 'creatingField' : 'idle',
},
// If we have a search in progress, we reexecute the search to update our result array
search: Boolean(state.search.term)
? {
Expand Down