From af1c2cf1a01f01b48638d7f5c4bb201c1724a43d Mon Sep 17 00:00:00 2001 From: Erik Rasmussen Date: Fri, 23 Jul 2021 10:12:22 +0200 Subject: [PATCH 1/2] Use custom hook to create callback constants --- src/useConstantCallback.js | 30 ++++++ src/useConstantCallback.test.js | 110 +++++++++++++++++++++ src/useField.js | 167 ++++++++++++-------------------- 3 files changed, 201 insertions(+), 106 deletions(-) create mode 100644 src/useConstantCallback.js create mode 100644 src/useConstantCallback.test.js diff --git a/src/useConstantCallback.js b/src/useConstantCallback.js new file mode 100644 index 0000000..b01556a --- /dev/null +++ b/src/useConstantCallback.js @@ -0,0 +1,30 @@ +// @flow +import * as React from 'react' + +/** + * Creates a callback, with dependencies, that will be + * instance === for the lifetime of the component. + */ +export default function useConstantCallback(callback, deps) { + // initialize refs on first render + const refs = deps.map(React.useRef) + // update refs on each additional render + deps.forEach((dep, index) => (refs[index].current = dep)) + // eslint-disable-next-line react-hooks/exhaustive-deps + const constant = React.useRef((...args) => { + // This if seems weird, but if args is [], then the + // first param will be the refs.map() + if (args && args.length) { + callback( + ...args, + refs.map(ref => ref.current) + ) + } else { + callback( + undefined, + refs.map(ref => ref.current) + ) + } + }, []) + return constant.current +} diff --git a/src/useConstantCallback.test.js b/src/useConstantCallback.test.js new file mode 100644 index 0000000..ca3f553 --- /dev/null +++ b/src/useConstantCallback.test.js @@ -0,0 +1,110 @@ +// @flow + +import * as React from 'react' +import { render, cleanup } from '@testing-library/react' +import '@testing-library/jest-dom/extend-expect' +import useConstantCallback from './useConstantCallback' + +describe('useConstantCallback', () => { + afterEach(cleanup) + + it('should give the same instance on every render, even as params/deps change', () => { + const callback = jest.fn() + const MyComponent = () => { + const [name, setName] = React.useState('John') + const [age, setAge] = React.useState(20) + const [isAdmin, setAdmin] = React.useState(false) + const constantCallback = useConstantCallback( + (time, [name, age, isAdmin]) => { + expect(typeof time).toBe('number') + callback(name, age, isAdmin) + }, + [name, age, isAdmin] + ) + const callbackRef = React.useRef(constantCallback) + expect(callbackRef.current).toBe(constantCallback) + return ( +
+ + + + +
+ ) + } + const { getByTestId } = render() + const call = getByTestId('call') + const changeName = getByTestId('changeName') + const changeAge = getByTestId('changeAge') + const changeAdmin = getByTestId('changeAdmin') + + expect(changeName).toHaveTextContent('John') + expect(changeAge).toHaveTextContent(20) + expect(changeAdmin).toHaveTextContent('No') + expect(callback).not.toHaveBeenCalled() + + call.click() + + expect(callback).toHaveBeenCalled() + expect(callback).toHaveBeenCalledTimes(1) + expect(callback.mock.calls[0][0]).toBe('John') + expect(callback.mock.calls[0][1]).toBe(20) + expect(callback.mock.calls[0][2]).toBe(false) + expect(changeName).toHaveTextContent('John') + expect(changeAge).toHaveTextContent(20) + expect(changeAdmin).toHaveTextContent('No') + + changeName.click() + + expect(callback).toHaveBeenCalledTimes(1) + expect(changeName).toHaveTextContent('Paul') + expect(changeAge).toHaveTextContent(20) + expect(changeAdmin).toHaveTextContent('No') + + call.click() + + expect(callback).toHaveBeenCalledTimes(2) + expect(callback.mock.calls[1][0]).toBe('Paul') + expect(callback.mock.calls[1][1]).toBe(20) + expect(callback.mock.calls[1][2]).toBe(false) + + changeAge.click() + + expect(callback).toHaveBeenCalledTimes(2) + expect(changeName).toHaveTextContent('Paul') + expect(changeAge).toHaveTextContent(25) + expect(changeAdmin).toHaveTextContent('No') + + call.click() + + expect(callback).toHaveBeenCalledTimes(3) + expect(callback.mock.calls[2][0]).toBe('Paul') + expect(callback.mock.calls[2][1]).toBe(25) + expect(callback.mock.calls[2][2]).toBe(false) + + changeAdmin.click() + + expect(callback).toHaveBeenCalledTimes(3) + expect(changeName).toHaveTextContent('Paul') + expect(changeAge).toHaveTextContent(25) + expect(changeAdmin).toHaveTextContent('Yes') + + call.click() + + expect(callback).toHaveBeenCalledTimes(4) + expect(callback.mock.calls[3][0]).toBe('Paul') + expect(callback.mock.calls[3][1]).toBe(25) + expect(callback.mock.calls[3][2]).toBe(true) + }) +}) diff --git a/src/useField.js b/src/useField.js index 862bafa..faf2dc2 100644 --- a/src/useField.js +++ b/src/useField.js @@ -13,6 +13,7 @@ import getValue from './getValue' import useForm from './useForm' import useLatest from './useLatest' import { addLazyFieldMetaState } from './getters' +import useConstantCallback from './useConstantCallback' const all: FieldSubscription = fieldSubscriptionItems.reduce((result, key) => { result[key] = true @@ -130,110 +131,6 @@ function useField( ] ) - const _valueRef = React.useRef(_value) - React.useEffect(() => { - _valueRef.current = _value - }) - - const stateRef = React.useRef(state) - React.useEffect(() => { - stateRef.current = state - }) - - const nameRef = React.useRef(name) - React.useEffect(() => { - nameRef.current = name - }) - - const typeRef = React.useRef(type) - React.useEffect(() => { - typeRef.current = type - }) - - const formatOnBlurRef = React.useRef(formatOnBlur) - React.useEffect(() => { - formatOnBlurRef.current = formatOnBlur - }) - - const formatRef = React.useRef(format) - React.useEffect(() => { - formatRef.current = format - }) - - const parseRef = React.useRef(parse) - React.useEffect(() => { - parseRef.current = parse - }) - - const formRef = React.useRef(form) - React.useEffect(() => { - formRef.current = form - }) - - const componentRef = React.useRef(component) - React.useEffect(() => { - componentRef.current = component - }) - - const handlers = { - onBlur: React.useCallback( - (event: ?SyntheticFocusEvent<*>) => { - stateRef.current.blur() - if (formatOnBlurRef.current) { - /** - * Here we must fetch the value directly from Final Form because we cannot - * trust that our `state` closure has the most recent value. This is a problem - * if-and-only-if the library consumer has called `onChange()` immediately - * before calling `onBlur()`, but before the field has had a chance to receive - * the value update from Final Form. - */ - const fieldState: any = formRef.current.getFieldState(stateRef.current.name) - stateRef.current.change(formatRef.current(fieldState.value, stateRef.current.name)) - } - }, - [] - ), - onChange: React.useCallback( - (event: SyntheticInputEvent<*> | any) => { - // istanbul ignore next - if (process.env.NODE_ENV !== 'production' && event && event.target) { - const targetType = event.target.type - const unknown = - ~['checkbox', 'radio', 'select-multiple'].indexOf(targetType) && - !typeRef.current && - componentRef.current !== 'select' - - const value: any = - targetType === 'select-multiple' ? stateRef.current.value : _valueRef.current - - if (unknown) { - console.error( - `You must pass \`type="${ - targetType === 'select-multiple' ? 'select' : targetType - }"\` prop to your Field(${nameRef.current}) component.\n` + - `Without it we don't know how to unpack your \`value\` prop - ${ - Array.isArray(value) ? `[${value}]` : `"${value}"` - }.` - ) - } - } - - const value: any = - event && event.target - ? getValue(event, stateRef.current.value, _valueRef.current, isReactNative) - : event - stateRef.current.change(parseRef.current(value, nameRef.current)) - }, - [] - ), - onFocus: React.useCallback( - (event: ?SyntheticFocusEvent<*>) => { - stateRef.current.focus() - }, - [] - ) - } - const meta = {} addLazyFieldMetaState(meta, state) const input: FieldInputProps = { @@ -258,7 +155,7 @@ function useField( return value }, get checked() { - let value = state.value; + let value = state.value if (type === 'checkbox') { value = format(value, name) if (_value === undefined) { @@ -271,7 +168,65 @@ function useField( } return undefined }, - ...handlers + onBlur: useConstantCallback( + (event: ?SyntheticFocusEvent<*>, [form, format, formatOnBlur, state]) => { + state.blur() + if (formatOnBlur) { + /** + * Here we must fetch the value directly from Final Form because we cannot + * trust that our `state` closure has the most recent value. This is a problem + * if-and-only-if the library consumer has called `onChange()` immediately + * before calling `onBlur()`, but before the field has had a chance to receive + * the value update from Final Form. + */ + const fieldState: any = form.getFieldState(state.name) + state.change(format(fieldState.value, state.name)) + } + }, + [form, format, formatOnBlur, state] + ), + onChange: useConstantCallback( + ( + event: SyntheticInputEvent<*> | any, + [component, name, parse, state, type, _value] + ) => { + // istanbul ignore next + if (process.env.NODE_ENV !== 'production' && event && event.target) { + const targetType = event.target.type + const unknown = + ~['checkbox', 'radio', 'select-multiple'].indexOf(targetType) && + !type && + component !== 'select' + + const value: any = + targetType === 'select-multiple' ? state.value : _value + + if (unknown) { + console.error( + `You must pass \`type="${ + targetType === 'select-multiple' ? 'select' : targetType + }"\` prop to your Field(${name}) component.\n` + + `Without it we don't know how to unpack your \`value\` prop - ${ + Array.isArray(value) ? `[${value}]` : `"${value}"` + }.` + ) + } + } + + const value: any = + event && event.target + ? getValue(event, state.value, _value, isReactNative) + : event + state.change(parse(value, name)) + }, + [component, name, parse, state, type, _value] + ), + onFocus: useConstantCallback( + (event: ?SyntheticFocusEvent<*>, [state]) => { + state.focus() + }, + [state] + ) } if (multiple) { From d77bbbf4c08a7f716a85593aaac37090ecb10aba Mon Sep 17 00:00:00 2001 From: Erik Rasmussen Date: Fri, 23 Jul 2021 13:59:37 +0200 Subject: [PATCH 2/2] Switched to @mpenney99's solution --- src/useConstantCallback.js | 34 ++++------- src/useConstantCallback.test.js | 11 ++-- src/useField.js | 100 ++++++++++++++------------------ 3 files changed, 58 insertions(+), 87 deletions(-) diff --git a/src/useConstantCallback.js b/src/useConstantCallback.js index b01556a..0651f8a 100644 --- a/src/useConstantCallback.js +++ b/src/useConstantCallback.js @@ -2,29 +2,15 @@ import * as React from 'react' /** - * Creates a callback, with dependencies, that will be - * instance === for the lifetime of the component. + * Creates a callback, even with closures, that will be + * instance === for the lifetime of the component, always + * calling the most recent version of the function and its + * closures. */ -export default function useConstantCallback(callback, deps) { - // initialize refs on first render - const refs = deps.map(React.useRef) - // update refs on each additional render - deps.forEach((dep, index) => (refs[index].current = dep)) - // eslint-disable-next-line react-hooks/exhaustive-deps - const constant = React.useRef((...args) => { - // This if seems weird, but if args is [], then the - // first param will be the refs.map() - if (args && args.length) { - callback( - ...args, - refs.map(ref => ref.current) - ) - } else { - callback( - undefined, - refs.map(ref => ref.current) - ) - } - }, []) - return constant.current +export default function useConstantCallback(callback) { + const ref = React.useRef(callback) + React.useEffect(() => { + ref.current = callback + }) + return React.useCallback((...args) => ref.current.apply(null, args), []) } diff --git a/src/useConstantCallback.test.js b/src/useConstantCallback.test.js index ca3f553..b8c9bc8 100644 --- a/src/useConstantCallback.test.js +++ b/src/useConstantCallback.test.js @@ -14,13 +14,10 @@ describe('useConstantCallback', () => { const [name, setName] = React.useState('John') const [age, setAge] = React.useState(20) const [isAdmin, setAdmin] = React.useState(false) - const constantCallback = useConstantCallback( - (time, [name, age, isAdmin]) => { - expect(typeof time).toBe('number') - callback(name, age, isAdmin) - }, - [name, age, isAdmin] - ) + const constantCallback = useConstantCallback(time => { + expect(typeof time).toBe('number') + callback(name, age, isAdmin) + }) const callbackRef = React.useRef(constantCallback) expect(callbackRef.current).toBe(constantCallback) return ( diff --git a/src/useField.js b/src/useField.js index faf2dc2..9f66363 100644 --- a/src/useField.js +++ b/src/useField.js @@ -168,64 +168,52 @@ function useField( } return undefined }, - onBlur: useConstantCallback( - (event: ?SyntheticFocusEvent<*>, [form, format, formatOnBlur, state]) => { - state.blur() - if (formatOnBlur) { - /** - * Here we must fetch the value directly from Final Form because we cannot - * trust that our `state` closure has the most recent value. This is a problem - * if-and-only-if the library consumer has called `onChange()` immediately - * before calling `onBlur()`, but before the field has had a chance to receive - * the value update from Final Form. - */ - const fieldState: any = form.getFieldState(state.name) - state.change(format(fieldState.value, state.name)) - } - }, - [form, format, formatOnBlur, state] - ), - onChange: useConstantCallback( - ( - event: SyntheticInputEvent<*> | any, - [component, name, parse, state, type, _value] - ) => { - // istanbul ignore next - if (process.env.NODE_ENV !== 'production' && event && event.target) { - const targetType = event.target.type - const unknown = - ~['checkbox', 'radio', 'select-multiple'].indexOf(targetType) && - !type && - component !== 'select' - - const value: any = - targetType === 'select-multiple' ? state.value : _value - - if (unknown) { - console.error( - `You must pass \`type="${ - targetType === 'select-multiple' ? 'select' : targetType - }"\` prop to your Field(${name}) component.\n` + - `Without it we don't know how to unpack your \`value\` prop - ${ - Array.isArray(value) ? `[${value}]` : `"${value}"` - }.` - ) - } - } + onBlur: useConstantCallback((event: ?SyntheticFocusEvent<*>) => { + state.blur() + if (formatOnBlur) { + /** + * Here we must fetch the value directly from Final Form because we cannot + * trust that our `state` closure has the most recent value. This is a problem + * if-and-only-if the library consumer has called `onChange()` immediately + * before calling `onBlur()`, but before the field has had a chance to receive + * the value update from Final Form. + */ + const fieldState: any = form.getFieldState(state.name) + state.change(format(fieldState.value, state.name)) + } + }), + onChange: useConstantCallback((event: SyntheticInputEvent<*> | any) => { + // istanbul ignore next + if (process.env.NODE_ENV !== 'production' && event && event.target) { + const targetType = event.target.type + const unknown = + ~['checkbox', 'radio', 'select-multiple'].indexOf(targetType) && + !type && + component !== 'select' const value: any = - event && event.target - ? getValue(event, state.value, _value, isReactNative) - : event - state.change(parse(value, name)) - }, - [component, name, parse, state, type, _value] - ), - onFocus: useConstantCallback( - (event: ?SyntheticFocusEvent<*>, [state]) => { - state.focus() - }, - [state] + targetType === 'select-multiple' ? state.value : _value + + if (unknown) { + console.error( + `You must pass \`type="${ + targetType === 'select-multiple' ? 'select' : targetType + }"\` prop to your Field(${name}) component.\n` + + `Without it we don't know how to unpack your \`value\` prop - ${ + Array.isArray(value) ? `[${value}]` : `"${value}"` + }.` + ) + } + } + + const value: any = + event && event.target + ? getValue(event, state.value, _value, isReactNative) + : event + state.change(parse(value, name)) + }), + onFocus: useConstantCallback((event: ?SyntheticFocusEvent<*>) => + state.focus() ) }