From db646f6c34fc9ea317dd4732049b5fa4e7f4bc5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Mon, 17 Aug 2020 15:20:14 +0200 Subject: [PATCH 1/6] Do not serialize field value on change + add test coverage --- .../components/use_field.test.tsx | 92 ++++++++++++++++++- .../forms/hook_form_lib/hooks/use_field.ts | 15 +-- .../forms/hook_form_lib/hooks/use_form.ts | 5 +- 3 files changed, 95 insertions(+), 17 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 f00beb470a9fc..4dcaf093fb336 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 @@ -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'; @@ -62,4 +63,91 @@ describe('', () => { 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 | 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) => { + formHook = _form; + }; + + const TestComp = ({ onForm }: { onForm: (form: FormHook) => void }) => { + const { form } = useForm({ defaultValue: { name: 'John' } }); + + useEffect(() => { + onForm(form); + }, [form]); + + return ( +
+ + + ); + }; + + 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 }) ?? ({} as MyForm); + 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() ?? ({} as MyForm); + 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(); + }); + }); }); 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 15ea99eb6cc3a..caf75b42598f5 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 @@ -118,15 +118,13 @@ export const useField = ( 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]); @@ -153,7 +151,6 @@ export const useField = ( } } }, [ - serializeOutput, valueChangeListener, errorDisplayDelay, path, @@ -442,13 +439,7 @@ export const useField = ( 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] diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts index 46b8958491e56..79f83081680a0 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts @@ -140,7 +140,7 @@ export function useForm( return Object.entries(fieldsRefs.current).reduce( (acc, [key, field]) => ({ ...acc, - [key]: field.__serializeOutput(), + [key]: field.value, }), {} as T ); @@ -233,8 +233,7 @@ export function useForm( 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] From 821dbde767953487cc0261dee39fdf9a2cb0b035 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Thu, 13 Aug 2020 10:46:57 +0200 Subject: [PATCH 2/6] [Form lib] submit(): don't get form data if the form is invalid We want to have the guarantee that when the serializers are called, the form is valid (all the fields value are valid). This would prevent for example a serializer to execute a JSON.parse() on a string that is not valid JSON. --- .../static/forms/hook_form_lib/hooks/use_form.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts index 79f83081680a0..696d156bc9a58 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts @@ -300,17 +300,17 @@ export function useForm( setSubmitting(true); const isFormValid = await validateAllFields(); - const formData = getFormData(); + const formData = isFormValid ? getFormData() : null; if (onSubmit) { - await onSubmit(formData, isFormValid!); + await onSubmit(formData ?? ({} as T), isFormValid!); } if (isUnmounted.current === false) { setSubmitting(false); } - return { data: formData, isValid: isFormValid! }; + return { data: formData ?? ({} as T), isValid: isFormValid! }; }, [validateAllFields, getFormData, onSubmit] ); From a7261d6e05b73d25f4e4f06bcccb258f334ea6b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Mon, 17 Aug 2020 16:26:24 +0200 Subject: [PATCH 3/6] Add test coverage to form.submit() with invalid fields --- .../hook_form_lib/hooks/use_form.test.tsx | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) 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 216c7974a9679..b2cc91152b571 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,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 { @@ -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 | null = null; + + const onFormHook = (_form: FormHook) => { + formHook = _form; + }; + + const TestComp = ({ onForm }: { onForm: (form: FormHook) => void }) => { + const { form } = useForm({ defaultValue: { username: 'initialValue' } }); + const validator: ValidationFunc = ({ value }) => { + if (value === 'wrongValue') { + return { message: 'Error on the field' }; + } + }; + + useEffect(() => { + onForm(form); + }, [form]); + + return ( +
+ + + ); + }; + + 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()', () => { From 6f74d9c78cabf70d8ac55d895ddb6b89dc5d5338 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Mon, 17 Aug 2020 16:56:05 +0200 Subject: [PATCH 4/6] Small refactor --- .../static/forms/hook_form_lib/hooks/use_form.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts index 696d156bc9a58..1f51b75a80b21 100644 --- a/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts +++ b/src/plugins/es_ui_shared/static/forms/hook_form_lib/hooks/use_form.ts @@ -300,17 +300,17 @@ export function useForm( setSubmitting(true); const isFormValid = await validateAllFields(); - const formData = isFormValid ? getFormData() : null; + const formData = isFormValid ? getFormData() : ({} as T); if (onSubmit) { - await onSubmit(formData ?? ({} as T), isFormValid!); + await onSubmit(formData, isFormValid!); } if (isUnmounted.current === false) { setSubmitting(false); } - return { data: formData ?? ({} as T), isValid: isFormValid! }; + return { data: formData, isValid: isFormValid! }; }, [validateAllFields, getFormData, onSubmit] ); From 3fb823b14c11c9dbd11c547030f03da5eab07cbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Tue, 18 Aug 2020 13:11:10 +0200 Subject: [PATCH 5/6] Fix regression in mappings editor forms --- .../fields/create_field/create_field.tsx | 24 ++++++++++++------- .../fields/edit_field/edit_field.tsx | 10 ++++---- .../edit_field/edit_field_header_form.tsx | 23 ++++++++++-------- .../components/mappings_editor/reducer.ts | 5 ++++ 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/create_field/create_field.tsx b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/create_field/create_field.tsx index ecaa40b398d08..ce5f2a60f5165 100644 --- a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/create_field/create_field.tsx +++ b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/create_field/create_field.tsx @@ -111,14 +111,17 @@ export const CreateField = React.memo(function CreateFieldComponent({ {/* Field subType (if any) */} - {({ type }) => ( - - )} + {({ type }) => { + const [fieldType] = type; + return ( + + ); + }} ); @@ -188,7 +191,10 @@ export const CreateField = React.memo(function CreateFieldComponent({ {({ type, subType }) => { - const ParametersForm = getParametersFormForType(type, subType); + const ParametersForm = getParametersFormForType( + type?.[0].value, + subType?.[0].value + ); if (!ParametersForm) { return null; diff --git a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/edit_field/edit_field.tsx b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/edit_field/edit_field.tsx index e6950ccfe253e..a9bbf008e5129 100644 --- a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/edit_field/edit_field.tsx +++ b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/edit_field/edit_field.tsx @@ -98,15 +98,15 @@ export const EditField = React.memo(({ form, field, allFields, exitEdit, updateF {({ 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 ( @@ -148,7 +148,7 @@ export const EditField = React.memo(({ form, field, allFields, exitEdit, updateF {({ type, subType }) => { - const ParametersForm = getParametersFormForType(type, subType); + const ParametersForm = getParametersFormForType(type?.[0].value, subType?.[0].value); if (!ParametersForm) { return null; diff --git a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/edit_field/edit_field_header_form.tsx b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/edit_field/edit_field_header_form.tsx index 5b969fa7ed827..b4b5bce21f768 100644 --- a/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/edit_field/edit_field_header_form.tsx +++ b/x-pack/plugins/index_management/public/application/components/mappings_editor/components/document_fields/fields/edit_field/edit_field_header_form.tsx @@ -36,15 +36,18 @@ export const EditFieldHeaderForm = React.memo( {/* Field subType (if any) */} - {({ type }) => ( - - )} + {({ type }) => { + const [fieldType] = type; + return ( + + ); + }} @@ -52,7 +55,7 @@ export const EditFieldHeaderForm = React.memo( {({ 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) { diff --git a/x-pack/plugins/index_management/public/application/components/mappings_editor/reducer.ts b/x-pack/plugins/index_management/public/application/components/mappings_editor/reducer.ts index 18a8270117ea4..2a8368c666859 100644 --- a/x-pack/plugins/index_management/public/application/components/mappings_editor/reducer.ts +++ b/x-pack/plugins/index_management/public/application/components/mappings_editor/reducer.ts @@ -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) ? { From a462239423e1dd123b4bd0b2e86b627f3ab7de82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Se=CC=81bastien=20Loix?= Date: Tue, 18 Aug 2020 14:11:15 +0200 Subject: [PATCH 6/6] Small refactor --- .../static/forms/hook_form_lib/components/use_field.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 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 4dcaf093fb336..eead90d2f75b7 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 @@ -127,7 +127,7 @@ describe('', () => { expect(serializer).not.toBeCalled(); expect(formatter).not.toBeCalled(); - let formData = formHook.getFormData({ unflatten: false }) ?? ({} as MyForm); + let formData = formHook.getFormData({ unflatten: false }); expect(formData.name).toEqual('John-deserialized'); await act(async () => { @@ -137,7 +137,7 @@ describe('', () => { 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() ?? ({} as MyForm); + formData = formHook.getFormData(); expect(serializer).toBeCalled(); expect(formData.name).toEqual('MIKE-serialized');