From 701955a70b0bf83092e09e5b174816fa77a3a1a3 Mon Sep 17 00:00:00 2001 From: Ryan Kim Date: Sun, 17 Nov 2019 22:45:31 -0600 Subject: [PATCH 1/2] Register only once for useField hook --- src/Field.js | 2 + src/ReactFinalForm.test.js | 203 +++++++++++++++++++++++++++++++++++-- src/types.js.flow | 1 + src/useField.js | 91 +++++++++-------- 4 files changed, 248 insertions(+), 49 deletions(-) diff --git a/src/Field.js b/src/Field.js index baab6f01..4e7706e8 100644 --- a/src/Field.js +++ b/src/Field.js @@ -16,6 +16,7 @@ const Field = ({ formatOnBlur, initialValue, isEqual, + keepFieldStateOnUnmount, multiple, name, parse, @@ -38,6 +39,7 @@ const Field = ({ formatOnBlur, initialValue, isEqual, + keepFieldStateOnUnmount, multiple, parse, subscription, diff --git a/src/ReactFinalForm.test.js b/src/ReactFinalForm.test.js index 5e38ac37..f92080ef 100644 --- a/src/ReactFinalForm.test.js +++ b/src/ReactFinalForm.test.js @@ -912,7 +912,8 @@ describe('ReactFinalForm', () => { it('should allow an alternative form api to be passed in', () => { const onSubmit = jest.fn() const form = createForm({ onSubmit: onSubmit }) - const formMock = jest.spyOn(form, 'registerField') + const registerFieldSpy = jest.spyOn(form, 'registerField') + const subscribeToFieldStateSpy = jest.spyOn(form, 'subscribeToFieldState') render(
{({ handleSubmit }) => ( @@ -922,14 +923,200 @@ describe('ReactFinalForm', () => { )}
) - expect(formMock).toHaveBeenCalled() - // called once on first render to get initial state, and then again to subscribe - expect(formMock).toHaveBeenCalledTimes(2) - expect(formMock.mock.calls[0][0]).toBe('name') - expect(formMock.mock.calls[0][2].active).toBe(true) // default subscription - expect(formMock.mock.calls[1][0]).toBe('name') - expect(formMock.mock.calls[1][2].active).toBe(true) // default subscription + // called once on first render to register only once + expect(registerFieldSpy).toHaveBeenCalledTimes(1) + expect(registerFieldSpy.mock.calls[0][0]).toBe('name') + expect(registerFieldSpy.mock.calls[0][1]).toBe(undefined) // no initial callback + expect(registerFieldSpy.mock.calls[0][2]).toBe(undefined) // no subscription + + // subscribe to field state once + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(1) + expect(subscribeToFieldStateSpy.mock.calls[0][0]).toBe('name') + expect(subscribeToFieldStateSpy.mock.calls[0][2].active).toBe(true) // default subscription + }) + + it('should keep field states when field is hidden with keepFieldStateOnUnmount option', () => { + const { getByTestId, getByText } = render( + + {hidden => ( +
+ {({ handleSubmit }) => ( + + {!hidden && ( + + v.toLowerCase() !== v ? 'SHOULD BE LOWERCASE' : undefined + } + > + {({ input, meta }) => ( + <> + + {meta.error} + + )} + + )} +
+ )} + + )} + + ) + + const nameField = getByTestId('name') + const errorElem = getByTestId('error') + expect(nameField).toHaveValue('erikras') + expect(errorElem).not.toHaveTextContent('SHOULD BE LOWERCASE') + + fireEvent.change(nameField, { target: { value: 'ERIKRAS' } }) + + expect(nameField).toHaveValue('ERIKRAS') + expect(errorElem).toHaveTextContent('SHOULD BE LOWERCASE') + + const toggleButton = getByText('Toggle') + // hide + fireEvent.click(toggleButton) + expect(nameField).not.toBeInTheDocument() + + // show + fireEvent.click(toggleButton) + expect(nameField).toHaveValue('ERIKRAS') + expect(errorElem).toHaveTextContent('SHOULD BE LOWERCASE') + }) + + it('should not re-register when hidden field becomes visible again with keepFieldStateOnUnmount option', () => { + const onSubmit = jest.fn() + const form = createForm({ onSubmit: onSubmit }) + const registerFieldSpy = jest.spyOn(form, 'registerField') + const subscribeToFieldStateSpy = jest.spyOn(form, 'subscribeToFieldState') + + const { getByTestId, getByText } = render( + + {hidden => ( +
+ {({ handleSubmit }) => ( + + {!hidden && ( + + )} + + )} + + )} +
+ ) + + const nameField = getByTestId('name') + const toggleButton = getByText('Toggle') + expect(registerFieldSpy).toHaveBeenCalledTimes(1) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(1) + + // hide + fireEvent.click(toggleButton) + expect(registerFieldSpy).toHaveBeenCalledTimes(1) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(1) + + // show + fireEvent.click(toggleButton) + expect(registerFieldSpy).toHaveBeenCalledTimes(1) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(2) + }) + + it('should re-register with the name prop change', () => { + const onSubmit = jest.fn() + const form = createForm({ onSubmit: onSubmit }) + const registerFieldSpy = jest.spyOn(form, 'registerField') + const subscribeToFieldStateSpy = jest.spyOn(form, 'subscribeToFieldState') + + const { getByTestId, getByText } = render( + + {isCat => ( +
+ {({ handleSubmit }) => ( + + + + )} + + )} +
+ ) + + const nameField = getByTestId('name') + const toggleButton = getByText('Toggle') + expect(registerFieldSpy).toHaveBeenCalledTimes(1) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(1) + + // change to the input field shouldn't trigger re-register + fireEvent.change(nameField, { target: { value: 'Jon' } }) + expect(registerFieldSpy).toHaveBeenCalledTimes(1) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(1) + + fireEvent.click(toggleButton) + expect(registerFieldSpy).toHaveBeenCalledTimes(2) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(2) + + fireEvent.click(toggleButton) + expect(registerFieldSpy).toHaveBeenCalledTimes(3) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(3) + }) + + it('should re-register with the name prop change with keepFieldStateOnUnmount', () => { + const onSubmit = jest.fn() + const form = createForm({ onSubmit: onSubmit }) + const registerFieldSpy = jest.spyOn(form, 'registerField') + const subscribeToFieldStateSpy = jest.spyOn(form, 'subscribeToFieldState') + + const { getByTestId, getByText } = render( + + {isCat => ( +
+ {({ handleSubmit }) => ( + + + + )} + + )} +
+ ) + + const nameField = getByTestId('name') + const toggleButton = getByText('Toggle') + expect(registerFieldSpy).toHaveBeenCalledTimes(1) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(1) + + // change to the input field shouldn't trigger re-register + fireEvent.change(nameField, { target: { value: 'Jon' } }) + expect(registerFieldSpy).toHaveBeenCalledTimes(1) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(1) + + fireEvent.click(toggleButton) + expect(registerFieldSpy).toHaveBeenCalledTimes(2) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(2) + + // this should change the name back to cat (or dog, whatever toggle toggles the toggle) + // since the states weren't removed, registration should not happen again, but subscription to field will + fireEvent.click(toggleButton) + expect(registerFieldSpy).toHaveBeenCalledTimes(2) + expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(3) }) it('should not destroy on unregister on initial unregister', () => { diff --git a/src/types.js.flow b/src/types.js.flow index 5784a865..a06d7143 100644 --- a/src/types.js.flow +++ b/src/types.js.flow @@ -94,6 +94,7 @@ export type UseFieldAutoConfig = { formatOnBlur?: boolean, initialValue?: any, isEqual?: (a: any, b: any) => boolean, + keepFieldStateOnUnmount?: boolean, multiple?: boolean, parse?: (value: any, name: string) => any, type?: string, diff --git a/src/useField.js b/src/useField.js index 48a52499..458f5e8c 100644 --- a/src/useField.js +++ b/src/useField.js @@ -39,6 +39,7 @@ function useField( format = defaultFormat, formatOnBlur, initialValue, + keepFieldStateOnUnmount = false, multiple, parse = defaultParse, subscription = all, @@ -50,13 +51,13 @@ function useField( const configRef = useLatest(config) - const register = (callback: FieldState => void) => + const register = () => // avoid using `state` const in any closures created inside `register` // because they would refer `state` from current execution context // whereas actual `state` would defined in the subsequent `useField` hook // execution // (that would be caused by `setState` call performed in `register` callback) - form.registerField(name, callback, subscription, { + form.registerField(name, undefined, undefined, { afterSubmit, beforeSubmit: () => { const { @@ -84,50 +85,58 @@ function useField( validateFields }) - const firstRender = React.useRef(true) - - // synchronously register and unregister to query field state for our subscription on first render - const [state, setState] = React.useState((): FieldState => { - let initialState: FieldState = {} - - // temporarily disable destroyOnUnregister - const destroyOnUnregister = form.destroyOnUnregister - form.destroyOnUnregister = false + const firstRenderRef = React.useRef(true) + const unregisterRef = React.useRef() + + const registerAndGetInitialState: () => FieldState = () => { + let initState + if (!keepFieldStateOnUnmount) { + unregisterRef.current && unregisterRef.current() + unregisterRef.current = undefined + } else { + initState = form.getFieldState(name) + } + + // if no initial state, register! + if (!initState) { + const unregisterFn = register() + // only set unregister function when keepFieldStateOnUnmount option is false + if (keepFieldStateOnUnmount === false) { + unregisterRef.current = unregisterFn + } + initState = form.getFieldState(name) + } - register(state => { - initialState = state - })() + return initState || {} + } - // return destroyOnUnregister to its original value - form.destroyOnUnregister = destroyOnUnregister + // register on first render + // this will initially check for existing field state from the form before trying to register + const [state, setState] = React.useState( + registerAndGetInitialState + ) - return initialState - }) + React.useEffect(() => { + // make sure this doesn't get triggered on first render + if (firstRenderRef.current === false) { + unregisterRef.current && unregisterRef.current() + setState(registerAndGetInitialState()) + } - React.useEffect( - () => - register(state => { - if (firstRender.current) { - firstRender.current = false - } else { - setState(state) - } - }), - // eslint-disable-next-line react-hooks/exhaustive-deps - [ + const unsubscribeFieldState = form.subscribeToFieldState( name, - data, - defaultValue, - // If we want to allow inline fat-arrow field-level validation functions, we - // cannot reregister field every time validate function !==. - // validate, - initialValue - // The validateFields array is often passed as validateFields={[]}, creating - // a !== new array every time. If it needs to be changed, a rerender/reregister - // can be forced by changing the key prop - // validateFields - ] - ) + setState, + subscription + ) + + firstRenderRef.current = false + + return () => { + unsubscribeFieldState() + unregisterRef.current && unregisterRef.current() + unregisterRef.current = undefined + } + }, [defaultValue, initialValue, name]) const handlers = { onBlur: React.useCallback( From ee225a0386c73c2c16e2ba46b530aaedea26523f Mon Sep 17 00:00:00 2001 From: Ryan Kim Date: Mon, 3 Feb 2020 08:37:18 -0600 Subject: [PATCH 2/2] Code quality fix --- src/ReactFinalForm.test.js | 1 - src/useField.js | 40 +++++++++++++++++++++----------------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/ReactFinalForm.test.js b/src/ReactFinalForm.test.js index f92080ef..0de19009 100644 --- a/src/ReactFinalForm.test.js +++ b/src/ReactFinalForm.test.js @@ -1014,7 +1014,6 @@ describe('ReactFinalForm', () => { ) - const nameField = getByTestId('name') const toggleButton = getByText('Toggle') expect(registerFieldSpy).toHaveBeenCalledTimes(1) expect(subscribeToFieldStateSpy).toHaveBeenCalledTimes(1) diff --git a/src/useField.js b/src/useField.js index 458f5e8c..7034441c 100644 --- a/src/useField.js +++ b/src/useField.js @@ -116,27 +116,31 @@ function useField( registerAndGetInitialState ) - React.useEffect(() => { - // make sure this doesn't get triggered on first render - if (firstRenderRef.current === false) { - unregisterRef.current && unregisterRef.current() - setState(registerAndGetInitialState()) - } + React.useEffect( + () => { + // make sure this doesn't get triggered on first render + if (firstRenderRef.current === false) { + unregisterRef.current && unregisterRef.current() + setState(registerAndGetInitialState()) + } - const unsubscribeFieldState = form.subscribeToFieldState( - name, - setState, - subscription - ) + const unsubscribeFieldState = form.subscribeToFieldState( + name, + setState, + subscription + ) - firstRenderRef.current = false + firstRenderRef.current = false - return () => { - unsubscribeFieldState() - unregisterRef.current && unregisterRef.current() - unregisterRef.current = undefined - } - }, [defaultValue, initialValue, name]) + return () => { + unsubscribeFieldState() + unregisterRef.current && unregisterRef.current() + unregisterRef.current = undefined + } + }, + // eslint-disable-next-line react-hooks/exhaustive-deps + [defaultValue, initialValue, name] + ) const handlers = { onBlur: React.useCallback(