From 388e0b2f2b63968cd60e3c1007b66b31de80ff03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20H=C3=B8egh?= Date: Wed, 4 Sep 2024 07:37:08 +0200 Subject: [PATCH] fix(Forms): enhance cleanup routine of fields (#3885) I noticed a couple of issues due to the fact that we mount/unmount parts without a effect dependency. It was about getting the right content in fields that where removed/added dynamically inside iterate while working on this PR #3877. But I think we should rather pull out these changed and submit by its own PR. I have not checked any examples/demos. But would love if someone checks it for me, especially the error handling in [field compositions](https://eufemia-git-fix-mount-unmount-routine-eufemia.vercel.app/uilib/extensions/forms/base-fields/Composition/) (FieldBlock with nested fields). But in overall, it "should not" have a negative effect on existing implementations. --- .../extensions/forms/DataContext/Context.ts | 3 +- .../forms/DataContext/Provider/Provider.tsx | 23 ++-- .../Provider/__tests__/Provider.test.tsx | 8 +- .../forms/FieldBlock/FieldBlock.tsx | 12 +- .../extensions/forms/hooks/useFieldProps.ts | 124 ++++++++++-------- .../__tests__/useUnmountEffect.test.ts | 29 ---- .../src/shared/helpers/useUnmountEffect.ts | 11 -- 7 files changed, 91 insertions(+), 119 deletions(-) delete mode 100644 packages/dnb-eufemia/src/shared/helpers/__tests__/useUnmountEffect.test.ts delete mode 100644 packages/dnb-eufemia/src/shared/helpers/useUnmountEffect.ts diff --git a/packages/dnb-eufemia/src/extensions/forms/DataContext/Context.ts b/packages/dnb-eufemia/src/extensions/forms/DataContext/Context.ts index 551e61830bd..74f9afd58df 100644 --- a/packages/dnb-eufemia/src/extensions/forms/DataContext/Context.ts +++ b/packages/dnb-eufemia/src/extensions/forms/DataContext/Context.ts @@ -110,7 +110,7 @@ export interface ContextState { skipFieldValidation?: boolean skipErrorCheck?: boolean }) => void - setFieldEventListener: ( + setFieldEventListener?: ( path: EventListenerCall['path'], type: EventListenerCall['type'], callback: EventListenerCall['callback'] @@ -154,7 +154,6 @@ export const defaultContextState: ContextState = { formState: undefined, setFormState: () => null, setSubmitState: () => null, - setFieldEventListener: () => null, handleSubmitCall: () => null, setShowAllErrors: () => null, handleMountField: () => null, diff --git a/packages/dnb-eufemia/src/extensions/forms/DataContext/Provider/Provider.tsx b/packages/dnb-eufemia/src/extensions/forms/DataContext/Provider/Provider.tsx index 3b66092cc00..93a4687d2b2 100644 --- a/packages/dnb-eufemia/src/extensions/forms/DataContext/Provider/Provider.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/DataContext/Provider/Provider.tsx @@ -29,7 +29,6 @@ import { import type { IsolationProviderProps } from '../../Form/Isolation/Isolation' import { debounce } from '../../../../shared/helpers' import FieldPropsProvider from '../../Form/FieldProps' -import useMountEffect from '../../../../shared/helpers/useMountEffect' import useUpdateEffect from '../../../../shared/helpers/useUpdateEffect' import { isAsync } from '../../../../shared/helpers/isAsync' import { useSharedState } from '../../../../shared/helpers/useSharedState' @@ -64,14 +63,18 @@ export interface Props * Unique ID to connect with a GlobalStatus */ globalStatusId?: string + /** + * Source data, will be used instead of defaultData, and leading to updates if changed after mount + */ + data?: Data /** * Default source data, only used if no other source is available, and not leading to updates if changed after mount */ defaultData?: Data /** - * Source data, will be used instead of defaultData, and leading to updates if changed after mount + * Empty data, used to clear the data set. */ - data?: Data + emptyData?: unknown /** * JSON Schema to validate the data against. */ @@ -178,6 +181,7 @@ export default function Provider( id, globalStatusId = 'main', defaultData, + emptyData, data, schema, onChange, @@ -241,7 +245,7 @@ export default function Provider( } else { delete hasVisibleErrorRef.current[path] } - forceUpdate() + forceUpdate() // Will rerender the whole form initially }, [] ) @@ -709,7 +713,7 @@ export default function Provider( storeInSession() } - forceUpdate() + forceUpdate() // Will rerender the whole form initially }, [ extendSharedData, @@ -829,14 +833,15 @@ export default function Provider( }, []) const clearData = useCallback(() => { - internalDataRef.current = clearedData as Data + internalDataRef.current = (emptyData ?? clearedData) as Data + if (id) { setSharedData?.(internalDataRef.current) } else { forceUpdate() } onClear?.() - }, [id, onClear, setSharedData]) + }, [emptyData, id, onClear, setSharedData]) /** * Shared logic dedicated to submit the whole form @@ -1084,14 +1089,14 @@ export default function Provider( } // - ajv validator routines - useMountEffect(() => { + useEffect(() => { if (schema) { ajvValidatorRef.current = ajvRef.current?.compile(schema) } // Validate the initial data validateData() - }) + }, [schema, validateData]) useUpdateEffect(() => { if (schema && schema !== cacheRef.current.schema) { cacheRef.current.schema = schema diff --git a/packages/dnb-eufemia/src/extensions/forms/DataContext/Provider/__tests__/Provider.test.tsx b/packages/dnb-eufemia/src/extensions/forms/DataContext/Provider/__tests__/Provider.test.tsx index cab8d95b955..8d73c819a68 100644 --- a/packages/dnb-eufemia/src/extensions/forms/DataContext/Provider/__tests__/Provider.test.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/DataContext/Provider/__tests__/Provider.test.tsx @@ -925,14 +925,11 @@ describe('DataContext.Provider', () => { data={{ fooBar: 'changed' }} onSubmitRequest={onSubmitRequest} > - + Submit ) - fireEvent.change(inputElement, { - target: { value: '2' }, - }) fireEvent.click(submitButton) expect(onSubmitRequest).toHaveBeenCalledTimes(2) @@ -2866,9 +2863,6 @@ describe('DataContext.Provider', () => { ) - fireEvent.change(inputElement, { - target: { value: '2' }, - }) fireEvent.click(submitButton) expect(onSubmitRequest).toHaveBeenCalledTimes(2) diff --git a/packages/dnb-eufemia/src/extensions/forms/FieldBlock/FieldBlock.tsx b/packages/dnb-eufemia/src/extensions/forms/FieldBlock/FieldBlock.tsx index fd7db412621..975b2592427 100644 --- a/packages/dnb-eufemia/src/extensions/forms/FieldBlock/FieldBlock.tsx +++ b/packages/dnb-eufemia/src/extensions/forms/FieldBlock/FieldBlock.tsx @@ -30,7 +30,6 @@ import { warn, } from '../../../shared/component-helper' import useId from '../../../shared/helpers/useId' -import useUnmountEffect from '../../../shared/helpers/useUnmountEffect' import { ComponentProps, FieldProps, @@ -355,10 +354,13 @@ function FieldBlock(props: Props) { } }, [errorProp, blockId, showFieldError, nestedFieldBlockContext]) - useUnmountEffect(() => () => { - mountedFieldsRef.current = {} - stateRecordRef.current = {} - }) + useEffect( + () => () => { + mountedFieldsRef.current = {} + stateRecordRef.current = {} + }, + [] + ) const mainClasses = classnames( 'dnb-forms-field-block', diff --git a/packages/dnb-eufemia/src/extensions/forms/hooks/useFieldProps.ts b/packages/dnb-eufemia/src/extensions/forms/hooks/useFieldProps.ts index f97dc7f3be2..945144ef145 100644 --- a/packages/dnb-eufemia/src/extensions/forms/hooks/useFieldProps.ts +++ b/packages/dnb-eufemia/src/extensions/forms/hooks/useFieldProps.ts @@ -1,4 +1,4 @@ -import { +import React, { useRef, useEffect, useContext, @@ -25,8 +25,6 @@ import FieldPropsContext from '../Form/FieldProps/FieldPropsContext' import { combineDescribedBy, warn } from '../../../shared/component-helper' import useId from '../../../shared/helpers/useId' import useUpdateEffect from '../../../shared/helpers/useUpdateEffect' -import useMountEffect from '../../../shared/helpers/useMountEffect' -import useUnmountEffect from '../../../shared/helpers/useUnmountEffect' import FieldBlockContext from '../FieldBlock/FieldBlockContext' import IterateElementContext from '../Iterate/IterateItemContext' import SectionContext from '../Form/Section/SectionContext' @@ -152,10 +150,13 @@ export default function useFieldProps( setFieldError: setFieldErrorDataContext, setFieldProps: setPropsDataContext, setHasVisibleError: setHasVisibleErrorDataContext, + handleMountField, + handleUnMountField, + setFieldEventListener, errors: dataContextErrors, showAllErrors, contextErrorMessages, - } = dataContext ?? {} + } = dataContext || {} const onChangeContext = dataContext?.props?.onChange const disabled = disabledProp ?? props.readOnly @@ -164,11 +165,11 @@ export default function useFieldProps( setFieldState: setFieldStateFieldBlock, showFieldError: showFieldErrorFieldBlock, mountedFieldsRef: mountedFieldsRefFieldBlock, - } = fieldBlockContext ?? {} + } = fieldBlockContext || {} const { handleChange: handleChangeIterateContext } = - iterateItemContext ?? {} - const { path: sectionPath, errorPrioritization } = sectionContext ?? {} - const { setFieldError, showBoundaryErrors } = fieldBoundaryContext ?? {} + iterateItemContext || {} + const { path: sectionPath, errorPrioritization } = sectionContext || {} + const { setFieldError, showBoundaryErrors } = fieldBoundaryContext || {} const hasPath = Boolean(pathProp) const { path, identifier, makeIteratePath } = usePath({ @@ -384,13 +385,13 @@ export default function useFieldProps( } } - const messagehasValues = Object.entries( - error.messageValues ?? {} + const messageHasValues = Object.entries( + error.messageValues || {} ).reduce((message, [key, value]) => { return message.replace(`{${key}}`, value) }, message) - error.message = messagehasValues + error.message = messageHasValues return error } @@ -1089,16 +1090,28 @@ export default function useFieldProps( // Put props into the surrounding data context as early as possible setPropsDataContext?.(identifier, props) - useMountEffect(() => { - dataContext?.handleMountField(identifier) + useEffect(() => { + // Mount procedure. + handleMountField(identifier) + + // Unmount procedure. + return () => { + handleUnMountField(identifier) + setFieldErrorDataContext?.(identifier, undefined) + setFieldError?.(identifier, undefined) + localErrorRef.current = undefined + } + }, [ + handleMountField, + handleUnMountField, + identifier, + setFieldError, + setFieldErrorDataContext, + ]) + + useEffect(() => { validateValue() - }) - useUnmountEffect(() => { - dataContext?.handleUnMountField(identifier) - setFieldErrorDataContext?.(identifier, undefined) - setFieldError?.(identifier, undefined) - localErrorRef.current = undefined - }) + }, [validateValue]) useUpdateEffect(() => { schemaValidatorRef.current = schema @@ -1212,6 +1225,7 @@ export default function useFieldProps( }, [ dataContext.data, dataContext.id, + defaultValue, hasPath, identifier, updateDataValueDataContext, @@ -1258,44 +1272,34 @@ export default function useFieldProps( }, [addToPool, callOnBlurValidator, callValidator, hasError, runPool]) // Validate/call validator functions during submit of the form - useMountEffect(() => { - dataContext?.setFieldEventListener?.( - identifier, - 'onSubmit', - onSubmitHandler - ) - }) + useEffect(() => { + setFieldEventListener?.(identifier, 'onSubmit', onSubmitHandler) + }, [identifier, onSubmitHandler, setFieldEventListener]) // Set the error in the field block context if this field is inside a field block - useMountEffect(() => { + useEffect(() => { if (inFieldBlock) { - if (errorProp) { - setFieldStateFieldBlock?.({ - identifier, - type: 'error', - content: errorProp, - showInitially: true, - show: true, - }) - } - if (warning) { - setFieldStateFieldBlock?.({ - identifier, - type: 'warning', - content: warning, - showInitially: true, - show: true, - }) - } - if (info) { - setFieldStateFieldBlock?.({ - identifier, - type: 'info', - content: info, - showInitially: true, - show: true, - }) - } + setFieldStateFieldBlock?.({ + identifier, + type: 'error', + content: errorProp, + showInitially: true, + show: true, + }) + setFieldStateFieldBlock?.({ + identifier, + type: 'warning', + content: warning, + showInitially: true, + show: true, + }) + setFieldStateFieldBlock?.({ + identifier, + type: 'info', + content: info, + showInitially: true, + show: true, + }) return () => { // Unmount procedure @@ -1304,7 +1308,15 @@ export default function useFieldProps( } } } - }) + }, [ + errorProp, + identifier, + inFieldBlock, + info, + mountedFieldsRefFieldBlock, + setFieldStateFieldBlock, + warning, + ]) const infoRef = useRef(info) const warningRef = useRef(warning) diff --git a/packages/dnb-eufemia/src/shared/helpers/__tests__/useUnmountEffect.test.ts b/packages/dnb-eufemia/src/shared/helpers/__tests__/useUnmountEffect.test.ts deleted file mode 100644 index 09494e00154..00000000000 --- a/packages/dnb-eufemia/src/shared/helpers/__tests__/useUnmountEffect.test.ts +++ /dev/null @@ -1,29 +0,0 @@ -import { renderHook } from '@testing-library/react' -import useUnmountEffect from '../useUnmountEffect' - -describe('useUnmountEffect', () => { - it('should unmount once and not be called on render or re-render', () => { - let wasUnmounted = 0 - - const effect = jest.fn(() => { - wasUnmounted += 1 - }) - const { rerender, unmount } = renderHook(() => - useUnmountEffect(effect) - ) - - expect(wasUnmounted).toBe(0) - - rerender() - rerender() - - expect(wasUnmounted).toBe(0) - - unmount() - unmount() - - expect(wasUnmounted).toBe(1) - - expect(effect).toHaveBeenCalledTimes(1) - }) -}) diff --git a/packages/dnb-eufemia/src/shared/helpers/useUnmountEffect.ts b/packages/dnb-eufemia/src/shared/helpers/useUnmountEffect.ts deleted file mode 100644 index ce06f1f9865..00000000000 --- a/packages/dnb-eufemia/src/shared/helpers/useUnmountEffect.ts +++ /dev/null @@ -1,11 +0,0 @@ -import { useEffect } from 'react' - -/** - * UseEffect that only run on the initial mount - */ -export default function useUnmountEffect(callback: () => void) { - useEffect(() => { - return callback - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []) -}