Skip to content

Commit

Permalink
[Form lib] Fix issue where serializer on fields are called on every c…
Browse files Browse the repository at this point in the history
…hange (#75166)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
sebelga and elasticmachine authored Aug 18, 2020
1 parent 0e28dae commit 196cb7f
Show file tree
Hide file tree
Showing 8 changed files with 200 additions and 43 deletions.
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

0 comments on commit 196cb7f

Please sign in to comment.