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.
dnbexperience · Sep 4, 2024 · 388e0b2 · 388e0b2
1 parent a5ff2a9
commit 388e0b2
Show file tree

Hide file tree

Showing 7 changed files with 91 additions and 119 deletions.
diff --git 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
@@ -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<Data extends JsonObject>
    * 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<Data extends JsonObject>(
     id,
     globalStatusId = 'main',
     defaultData,
+    emptyData,
     data,
     schema,
     onChange,
@@ -241,7 +245,7 @@ export default function Provider<Data extends JsonObject>(
       } else {
         delete hasVisibleErrorRef.current[path]
       }
-      forceUpdate()
+      forceUpdate() // Will rerender the whole form initially
     },
     []
   )
@@ -709,7 +713,7 @@ export default function Provider<Data extends JsonObject>(
         storeInSession()
       }
 
-      forceUpdate()
+      forceUpdate() // Will rerender the whole form initially
     },
     [
       extendSharedData,
@@ -829,14 +833,15 @@ export default function Provider<Data extends JsonObject>(
   }, [])
 
   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<Data extends JsonObject>(
   }
 
   // - 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
@@ -925,14 +925,11 @@ describe('DataContext.Provider', () => {
           data={{ fooBar: 'changed' }}
           onSubmitRequest={onSubmitRequest}
         >
-          <Field.Number path="/fooBar" minimum={3} />
+          <Field.Number path="/fooBar" required />
           <Form.SubmitButton>Submit</Form.SubmitButton>
         </DataContext.Provider>
       )
 
-      fireEvent.change(inputElement, {
-        target: { value: '2' },
-      })
       fireEvent.click(submitButton)
 
       expect(onSubmitRequest).toHaveBeenCalledTimes(2)
@@ -2866,9 +2863,6 @@ describe('DataContext.Provider', () => {
         </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
@@ -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
@@ -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<Value, EmptyValue, Props>(
     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<Value, EmptyValue, Props>(
     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<Value, EmptyValue, Props>(
           }
         }
 
-        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<Value, EmptyValue, Props>(
   // 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<Value, EmptyValue, Props>(
   }, [
     dataContext.data,
     dataContext.id,
+    defaultValue,
     hasPath,
     identifier,
     updateDataValueDataContext,
@@ -1258,44 +1272,34 @@ export default function useFieldProps<Value, EmptyValue, Props>(
   }, [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<Value, EmptyValue, Props>(
         }
       }
     }
-  })
+  }, [
+    errorProp,
+    identifier,
+    inFieldBlock,
+    info,
+    mountedFieldsRefFieldBlock,
+    setFieldStateFieldBlock,
+    warning,
+  ])
 
   const infoRef = useRef<React.ReactNode>(info)
   const warningRef = useRef<React.ReactNode>(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
diff --git a/packages/dnb-eufemia/src/shared/helpers/useUnmountEffect.ts b/packages/dnb-eufemia/src/shared/helpers/useUnmountEffect.ts