From 0d60dcefe5c49330063d30131534f729c668be99 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 4 May 2022 01:21:55 -0500 Subject: [PATCH 1/3] [@mantine/form] use-form: Make functions referentially stable --- docs/src/docs/form/use-form.mdx | 2 +- src/mantine-form/src/use-form.ts | 243 ++++++++++++++++++------------- 2 files changed, 142 insertions(+), 103 deletions(-) diff --git a/docs/src/docs/form/use-form.mdx b/docs/src/docs/form/use-form.mdx index 35e7dee29cc..d4f512d9bb8 100644 --- a/docs/src/docs/form/use-form.mdx +++ b/docs/src/docs/form/use-form.mdx @@ -42,7 +42,7 @@ yarn add @mantine/form - `schema` (optional) – [zod](https://www.npmjs.com/package/zod), [joi](https://www.npmjs.com/package/joi) or [yup](https://www.npmjs.com/package/yup) validation schema, if provided `validate` is ignored - `initialErrors` (optional) – initial form errors, object of React nodes -Hook returns an object with the following properties: +Hook returns an object with the following properties (the functions are all referentially stable): - `values` – current form values - `setValues` – handler to set all form values diff --git a/src/mantine-form/src/use-form.ts b/src/mantine-form/src/use-form.ts index 4c52e2c2f3c..68acd520d11 100644 --- a/src/mantine-form/src/use-form.ts +++ b/src/mantine-form/src/use-form.ts @@ -1,4 +1,4 @@ -import { useState } from 'react'; +import { useState, useCallback, useRef, useEffect } from 'react'; import { formList, isFormList, FormList } from './form-list/form-list'; import { validateValues, validateFieldValue } from './validate-values/validate-values'; import { filterErrors } from './filter-errors/filter-errors'; @@ -63,6 +63,23 @@ export interface UseFormReturnType { ) => GetInputProps; } +/** + * Creates a ref that stays up to date with the state of the param + * Used to help with all the useCallback signatures in useForm + * + * @param value The value to clone in the ref + * @returns The ref which clones the value + */ +function useStateRef(value: T): React.MutableRefObject { + const ref = useRef(value); + + useEffect(() => { + ref.current = value; + }, [value]); + + return ref; +} + export function useForm({ initialValues, initialErrors, @@ -70,39 +87,49 @@ export function useForm({ schema, }: UseFormInput): UseFormReturnType { const [errors, setErrors] = useState(filterErrors(initialErrors)); + const errorsRef = useStateRef(errors); const [values, setValues] = useState(initialValues); + const valuesRef = useStateRef(values); + const initialValuesRef = useStateRef(initialValues); + const rulesRef = useStateRef(rules); + const schemaRef = useStateRef(schema); - const clearErrors = () => setErrors({}); - const setFieldError = (field: keyof T, error: React.ReactNode) => - setErrors((current) => ({ ...current, [field]: error })); + const clearErrors = useCallback(() => setErrors({}), []); + const setFieldError = useCallback( + (field: keyof T, error: React.ReactNode) => + setErrors((current) => ({ ...current, [field]: error })), + [] + ); - const clearFieldError = (field: keyof T) => - setErrors((current) => { - const clone: any = { ...current }; - delete clone[field]; - return clone; - }); + const clearFieldError = useCallback( + (field: keyof T) => + setErrors((current) => { + const clone: any = { ...current }; + delete clone[field]; + return clone; + }), + [] + ); - const setFieldValue = (field: K, value: V) => { + const setFieldValue = useCallback((field: K, value: V) => { setValues((currentValues) => ({ ...currentValues, [field]: value })); clearFieldError(field); - }; + }, []); - const setListItem = ( - field: K, - index: number, - value: V[K][number] - ) => { - const list = values[field]; - if (isFormList(list) && list[index] !== undefined) { - const cloned = [...list]; - cloned[index] = value; - setFieldValue(field, formList(cloned) as any); - } - }; + const setListItem = useCallback( + (field: K, index: number, value: V[K][number]) => { + const list = valuesRef.current[field]; + if (isFormList(list) && list[index] !== undefined) { + const cloned = [...list]; + cloned[index] = value; + setFieldValue(field, formList(cloned) as any); + } + }, + [] + ); - const removeListItem = (field: K, indices: number[] | number) => { - const list = values[field]; + const removeListItem = useCallback((field: K, indices: number[] | number) => { + const list = valuesRef.current[field]; if (isFormList(list)) { setFieldValue( @@ -114,107 +141,119 @@ export function useForm({ ) as any ); } - }; + }, []); - const addListItem = (field: K, payload: V[number]) => { - const list = values[field]; + const addListItem = useCallback( + (field: K, payload: V[number]) => { + const list = valuesRef.current[field]; - if (isFormList(list)) { - setFieldValue(field, formList([...list, payload]) as any); - } - }; + if (isFormList(list)) { + setFieldValue(field, formList([...list, payload]) as any); + } + }, + [] + ); - const reorderListItem = ( - field: K, - { from, to }: { from: number; to: number } - ) => { - const list = values[field]; + const reorderListItem = useCallback( + (field: K, { from, to }: { from: number; to: number }) => { + const list = valuesRef.current[field]; - if (isFormList(list) && list[from] !== undefined && list[to] !== undefined) { - const cloned = [...list]; - const item = list[from]; + if (isFormList(list) && list[from] !== undefined && list[to] !== undefined) { + const cloned = [...list]; + const item = list[from]; - cloned.splice(from, 1); - cloned.splice(to, 0, item); - setFieldValue(field, formList(cloned) as any); - } - }; + cloned.splice(from, 1); + cloned.splice(to, 0, item); + setFieldValue(field, formList(cloned) as any); + } + }, + [] + ); - const validate = () => { - const results = validateValues(schema || rules, values); + const validate = useCallback(() => { + const results = validateValues(schemaRef.current || rulesRef.current, valuesRef.current); setErrors(results.errors); return results; - }; + }, []); - const validateField = (field: string) => { - const results = validateFieldValue(field, schema || rules, values); + const validateField = useCallback((field: string) => { + const results = validateFieldValue( + field, + schemaRef.current || rulesRef.current, + valuesRef.current + ); results.hasError ? setFieldError(field, results.error) : clearFieldError(field); return results; - }; + }, []); - const onSubmit = + const onSubmit = useCallback( (handleSubmit: (values: T, event: React.FormEvent) => void) => (event: React.FormEvent) => { event.preventDefault(); const results = validate(); - !results.hasErrors && handleSubmit(values, event); - }; + !results.hasErrors && handleSubmit(valuesRef.current, event); + }, + [] + ); - const reset = () => { - setValues(initialValues); + const reset = useCallback(() => { + setValues(initialValuesRef.current); clearErrors(); - }; + }, []); - const getInputProps = < - K extends keyof T, - U extends T[K], - L extends GetInputPropsFieldType = 'input' - >( - field: K, - { type, withError = true }: { type?: L; withError?: boolean } = {} - ): GetInputProps => { - const value = values[field]; - const onChange = getInputOnChange((val: U) => setFieldValue(field, val)) as any; + const getInputProps = useCallback( + ( + field: K, + { type, withError = true }: { type?: L; withError?: boolean } = {} + ): GetInputProps => { + const value = valuesRef.current[field]; + const onChange = getInputOnChange((val: U) => setFieldValue(field, val)) as any; - const payload: any = type === 'checkbox' ? { checked: value, onChange } : { value, onChange }; + const payload: any = type === 'checkbox' ? { checked: value, onChange } : { value, onChange }; - if (withError && errors[field as any]) { - payload.error = errors[field as any]; - } + if (withError && errorsRef.current[field as any]) { + payload.error = errorsRef.current[field as any]; + } - return payload as any; - }; + return payload as any; + }, + [] + ); - const getListInputProps = < - K extends keyof T, - U extends T[K][number], - LK extends keyof U, - L extends GetInputPropsFieldType = 'input' - >( - field: K, - index: number, - listField: LK, - { type, withError = true }: { type?: L; withError?: boolean } = {} - ): GetInputProps => { - const list = values[field]; - - if (isFormList(list) && list[index] && listField in list[index]) { - const listValue = list[index]; - const value = listValue[listField]; - const onChange = getInputOnChange((val: U[LK]) => - setListItem(field, index, { ...listValue, [listField]: val }) - ) as any; - const payload: any = type === 'checkbox' ? { checked: value, onChange } : { value, onChange }; - const error = errors[getErrorPath([field, index, listField])]; + const getListInputProps = useCallback( + < + K extends keyof T, + U extends T[K][number], + LK extends keyof U, + L extends GetInputPropsFieldType = 'input' + >( + field: K, + index: number, + listField: LK, + { type, withError = true }: { type?: L; withError?: boolean } = {} + ): GetInputProps => { + const list = valuesRef.current[field]; - if (withError && error) { - payload.error = error; - } + if (isFormList(list) && list[index] && listField in list[index]) { + const listValue = list[index]; + const value = listValue[listField]; + const onChange = getInputOnChange((val: U[LK]) => + setListItem(field, index, { ...listValue, [listField]: val }) + ) as any; + const payload: any = + type === 'checkbox' ? { checked: value, onChange } : { value, onChange }; + const error = errorsRef.current[getErrorPath([field, index, listField])]; - return payload; - } + if (withError && error) { + payload.error = error; + } - return {} as any; - }; + return payload; + } + + return {} as any; + }, + [] + ); return { values, From 2bedeeddb98d12a33f29240291c9110b6026d8ec Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 4 May 2022 15:00:10 -0500 Subject: [PATCH 2/3] Address review comments --- src/mantine-form/src/use-form.ts | 33 +++++-------------- .../src/use-prop-ref/use-prop-ref.ts | 11 +++++++ .../src/use-state-ref/use-state-ref.ts | 14 ++++++++ 3 files changed, 33 insertions(+), 25 deletions(-) create mode 100644 src/mantine-form/src/use-prop-ref/use-prop-ref.ts create mode 100644 src/mantine-form/src/use-state-ref/use-state-ref.ts diff --git a/src/mantine-form/src/use-form.ts b/src/mantine-form/src/use-form.ts index 68acd520d11..f8a1c4f151f 100644 --- a/src/mantine-form/src/use-form.ts +++ b/src/mantine-form/src/use-form.ts @@ -1,9 +1,11 @@ -import { useState, useCallback, useRef, useEffect } from 'react'; +import { useCallback } from 'react'; import { formList, isFormList, FormList } from './form-list/form-list'; import { validateValues, validateFieldValue } from './validate-values/validate-values'; import { filterErrors } from './filter-errors/filter-errors'; import { getInputOnChange } from './get-input-on-change/get-input-on-change'; import { getErrorPath } from './get-error-path/get-error-path'; +import { useStateRef } from './use-state-ref/use-state-ref'; +import { usePropRef } from './use-prop-ref/use-prop-ref'; import type { FormErrors, FormRules, @@ -63,36 +65,17 @@ export interface UseFormReturnType { ) => GetInputProps; } -/** - * Creates a ref that stays up to date with the state of the param - * Used to help with all the useCallback signatures in useForm - * - * @param value The value to clone in the ref - * @returns The ref which clones the value - */ -function useStateRef(value: T): React.MutableRefObject { - const ref = useRef(value); - - useEffect(() => { - ref.current = value; - }, [value]); - - return ref; -} - export function useForm({ initialValues, initialErrors, validate: rules, schema, }: UseFormInput): UseFormReturnType { - const [errors, setErrors] = useState(filterErrors(initialErrors)); - const errorsRef = useStateRef(errors); - const [values, setValues] = useState(initialValues); - const valuesRef = useStateRef(values); - const initialValuesRef = useStateRef(initialValues); - const rulesRef = useStateRef(rules); - const schemaRef = useStateRef(schema); + const [errors, setErrors, errorsRef] = useStateRef(filterErrors(initialErrors)); + const [values, setValues, valuesRef] = useStateRef(initialValues); + const initialValuesRef = usePropRef(initialValues); + const rulesRef = usePropRef(rules); + const schemaRef = usePropRef(schema); const clearErrors = useCallback(() => setErrors({}), []); const setFieldError = useCallback( diff --git a/src/mantine-form/src/use-prop-ref/use-prop-ref.ts b/src/mantine-form/src/use-prop-ref/use-prop-ref.ts new file mode 100644 index 00000000000..f708a26374f --- /dev/null +++ b/src/mantine-form/src/use-prop-ref/use-prop-ref.ts @@ -0,0 +1,11 @@ +import React, { useRef, useEffect } from 'react'; + +export function usePropRef(value: T): React.MutableRefObject { + const ref = useRef(value); + + useEffect(() => { + ref.current = value; + }, [value]); + + return ref; +} diff --git a/src/mantine-form/src/use-state-ref/use-state-ref.ts b/src/mantine-form/src/use-state-ref/use-state-ref.ts new file mode 100644 index 00000000000..c3ecb698431 --- /dev/null +++ b/src/mantine-form/src/use-state-ref/use-state-ref.ts @@ -0,0 +1,14 @@ +import React, { useRef, useState, useEffect } from 'react'; + +export function useStateRef( + value: T +): [T, React.Dispatch>, React.MutableRefObject] { + const [state, setState] = useState(value); + const ref = useRef(state); + + useEffect(() => { + ref.current = state; + }, [state]); + + return [state, setState, ref]; +} From 0a0c074bdc4d2f56e736183677e7e553cb9df185 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Wed, 4 May 2022 15:19:57 -0500 Subject: [PATCH 3/3] Add unit test for stable functions --- .../use-form.stable-functions.test.ts | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 src/mantine-form/src/use-form.test/use-form.stable-functions.test.ts diff --git a/src/mantine-form/src/use-form.test/use-form.stable-functions.test.ts b/src/mantine-form/src/use-form.test/use-form.stable-functions.test.ts new file mode 100644 index 00000000000..e6e369c9c82 --- /dev/null +++ b/src/mantine-form/src/use-form.test/use-form.stable-functions.test.ts @@ -0,0 +1,26 @@ +import { act, renderHook } from '@testing-library/react-hooks'; +import { useForm } from '../index'; + +describe('@mantine/form/use-form stable functions', () => { + it('keeps returned functions stable', () => { + const hook = renderHook(() => + useForm({ initialValues: { banana: 'initial banana', orange: 20, apple: { nested: true } } }) + ); + + const originalFunctions = { + ...hook.result.current, + }; + delete originalFunctions.values; + delete originalFunctions.errors; + + act(() => + hook.result.current.setValues({ + banana: 'modified banana', + orange: 21, + apple: { nested: false }, + }) + ); + + expect(hook.result.current).toMatchObject(originalFunctions); + }); +});