Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible perf issues with useField helpers #2268

Open
3nvi opened this issue Feb 3, 2020 · 31 comments
Open

Possible perf issues with useField helpers #2268

3nvi opened this issue Feb 3, 2020 · 31 comments
Labels

Comments

@3nvi
Copy link

3nvi commented Feb 3, 2020

🚀 Feature request

Since v2.1.0 we can do this:

const MyInput = (props) => {
 const [field, meta, helpers] = useField(props)
 const handleThing = () => {
    helpers.setValue('boop') 
 }
 // ...
}

Unfortunately, the helpers object (and the helpers.setValue) gets a different reference on every form update, regardless of whether the field has been updated or not. This means that if a single field got updated, then every other field that utilizes helpers.setValue as a prop value will have to re-render as well, regardless of whether it implements React.memo or not. For example the following component will always re-render on every form update:

const ComboboxMemo = React.memo(Combobox);

const MyCombobox = (props) => {
 const [field, meta, { setValue }]= useField(props.name);
 return <ComboboxMemo onChange={setValue} />
}

This issue is similar to 1804

Current Behavior

The helpers object and specifically the helpers.setValue gets a different reference on every form update

Desired Behavior

The helpers.setValue should only update when the value of the related form field got updated

Suggested Solution

There needs to be some sort of memoization based on the field's value, unrelated to the rest of the form. I haven't gotten into the v2 release, so I can't suggest something.

Who does this impact? Who is this for?

People that utilize setValue in component props and people that use heavy/big forms with multiple fields

@jaredpalmer
Copy link
Owner

I noticed this too. Not great.

@djsilcock
Copy link

Came here with this exact issue - the helper functions change on every render so my otherwise memoised component rerenders because the helper functions are different. Haven't hacked around to check yet, but could the helper functions be memoised in useField?

return [
getFieldProps(props),
getFieldMeta(fieldName),
useMemo(() => getFieldHelpers(fieldName), [getFieldHelpers, fieldName]) ,
];

@johnrom
Copy link
Collaborator

johnrom commented Feb 7, 2020

If we change this, users may have depended on the helpers changing instead of depending on values changing. Fixing this would be a breaking change. We should note it somewhere.

const formik = useFormikContext();
const [field, meta, { setValue }] = useField('secondaryField');
React.useEffect(() => {
    setValue(formik.values.firstField + 1);
}, [setValue]); // whoops, no formik.values

It should be an eventCallback though.

@3nvi
Copy link
Author

3nvi commented Feb 7, 2020

users may have depended on the helpers

I understand and agree. It would be rare though, since the setValue changes whenever the value changes. So I wouldn't expect to see users doing this:

React.useEffect(() => {
    setValue(formik.values.firstField + 1);
}, [setValue]); // whoops, no formik.values

instead of this

React.useEffect(() => {
    setValue(formik.values.firstField + 1);
}, [formik.values.firstField]);

since they would be capitalizing on an undocumented side-effect of the way setValue was declared.

@johnrom
Copy link
Collaborator

johnrom commented Feb 7, 2020

Definitely not expected. Just want to make sure it's noted in the release notes on the off-chance someone gets hit by it. There are some moments where I've sadly had to disable exhaustive-deps :(

@rafaelcavalcante
Copy link

rafaelcavalcante commented Feb 7, 2020

I just noticed that the methods between meta and helpers are inverted here.

Screen Shot 2020-02-07 at 14 34 57

@johnrom
Copy link
Collaborator

johnrom commented Feb 7, 2020

@rafaelcavalcante , useField returns a tuple, which means the values are in the same order no matter what you name them. I'd guess you wrote something like const [field, helpers, meta] = useField(name);, but it must be in the order [field, meta, helpers], or even [whatever, you, want], want.setValue().

@anant-singh
Copy link

Is there a resolution in works for this or we just have to turn off exhaustive-deps and not put helpers in the dependency array?

@johnrom
Copy link
Collaborator

johnrom commented Feb 11, 2020

@anant-singh I don't think anyone's actively working on this if you'd like to open a PR.

@terebentina
Copy link

terebentina commented Feb 28, 2020

The same problem with getFieldProps and setFieldValue when loaded from context:
const { getFieldProps, setFieldValue } = useFormikContext();

Later edit: setFieldValue has no problem, it's just getFieldProps that does.
Just for reference, this is how I am using them:

const { getFieldProps, setFieldValue } = useFormikContext();

const foo = useMemo(
  () => {
    ...
  },
  [getFieldProps, setFieldValue]
);

foo gets recreated every time with the above. Removing getFieldProps from the memo deps array keeps foo memoized (not a fix, I know, but this is just to explain how I got to this conclusion).

@priley86
Copy link

Noticing this issue as well, and it's causing several re-renders. Is there an accepted workaround at this point if using useField? Will certainly watch this one...

@mayorandrew
Copy link

Stumbled upon this issue and wrote myself a little helper hook to improve performance. Maybe it will help someone:

function useFieldFast(props) {
  const [field, meta, helpers] = useField(props);

  const latestRef = useRef({});

  // On every render save newest helpers to latestRef
  latestRef.current.setValue = helpers.setValue;
  latestRef.current.setTouched = helpers.setTouched;
  latestRef.current.setError = helpers.setError;

  // On the first render create new function which will never change
  // but call newest helper function
  if (!latestRef.current.helpers) {
    latestRef.current.helpers = {
      setValue: (...args) => latestRef.current.setValue(...args),
      setTouched: (...args) => latestRef.current.setTouched(...args),
      setError: (...args) => latestRef.current.setError(...args)
    };
  }

  return [field, meta, latestRef.current.helpers];
}

Still I think it would be beneficial if that was handled by the library. Like when you use useState, setState never changes.

@sibelius
Copy link

@mayorandrew do you have a codesanbox or some perf showing your finds?

@mayorandrew
Copy link

@sibelius I do not have a codesandbox with an example. You can just plug my hook and use that in place of useField.

I used that in my project (not public) and amount of rerenders declined dramatically. I.e. previously I would have every field re-render on every other field change which cause cascade of expensive input component re-renders like react-select. By making helper callbacks constant, I am allowing React.memo to cut off expensive subtrees.

@sibelius
Copy link

so you use your own hook and also React.memo?

@mayorandrew
Copy link

mayorandrew commented Mar 25, 2020

Yes. According to my source code and issue digging, in current formik architecture it is nearly impossible to make useField re-render that component only when the field actualy needs it (e.g. when value or something else related changes). So I am making wrappers for all my fields and rely on React.memo to prevent deeper rerenders, like so:

const MySelect = React.memo((props) => ... )

function FormikSelect(props) {
  const [field, meta, helpers] = useFieldFast(props);
  return <MySelect value={field.value} onChange={helpers.setValue} />;
}

Edit: fixed typo when passing helpers.setValue to onChange

@johnrom
Copy link
Collaborator

johnrom commented Mar 26, 2020

In terms of userland solutions, the above looks good!

In terms of solving this in Formik, we'd need to switch to useEventCallbacks for these helpers and if they access the underlying values (formik.values, formik.touched, etc), need to use refs of those values instead of depending on state, or use the formik reducer to mitigate race conditions.

@sibelius
Copy link

can't we use something similar to redux behavior? subscription based approach?

@stale stale bot added the stale label Jun 14, 2020
@WegDamit
Copy link

i try to reperoduce @mayorandrew approach and in the code:

const MySelect = React.memo((props) => ... )

function FormikSelect(props) {
  const [field, meta, helpers] = useFieldFast(props);
  return <MySelect value={field.value} onChange={meta.setValue} />;
}

Shouldn't it be helpers.setValuein the onChange prop?

And as i couldn't get it working somebody has a small working example of that stuff...?

@mayorandrew
Copy link

@WegDamit you are right, my typo. It should be helpers.setValue. I will fix it now.

I have created a demo sandbox here:
https://codesandbox.io/s/formik-fast-field-p7szk?file=/src/App.js

@drewgingerich
Copy link

@mayorandrew Your hook was very helpful, thank you! You seriously saved my butt, helping me understand the problem and realize what's possible.

In the project I'm on atm I noticed that OnBlur or OnChange in field was also changing every time any value was updated (I should probably figure out why). I updated your hook to also make static shims for OnBlur and OnChange, but still update when value or name change. Here are my updates in case they help anyone:

import { useRef } from 'react'
import { useField } from 'formik'

export default (props) => {
	const [field, meta, helpers] = useField(props)

	const helperRef = useRef()
	helperRef.current = helpers

	const helperShim = useRef({
		setValue: (args) => helperRef.current.setValue(args),
		setTouched: (args) => helperRef.current.setTouched(args),
		setError: (args) => helperRef.current.setError(args),
	})

	const fieldRef = useRef()
	fieldRef.current = field

	const makeFieldShim = ({ name, value }) => ({
		name,
		value,
		onBlur: (args) => fieldRef.current.onBlur(args),
		onChange: (args) => fieldRef.current.onChange(args),
	})

	const fieldShim = useRef(makeFieldShim(field))

	if (
		fieldShim.current.value !== field.value ||
		fieldShim.current.name !== field.name
	) {
		fieldShim.current = makeFieldShim(field)
	}

	return [fieldShim.current, meta, helperShim.current]
}

I use this hook in a HOC that mimics Formik's <Field />: the component passed to the as prop is handed all the Formik goodies as props, but is also automatically wrapped with React.memo().

import React, { useRef } from 'react'
import useSmartField from 'form/smart-field-hook'

export default (props) => {
	const { name, as, children, ...elementProps } = props
	const [field, meta, helpers] = useSmartField(name)

	const element = useRef({ cached: as, memoized: React.memo(as) })

	return React.createElement(
		element.current.memoized,
		{ ...elementProps, field, ...meta, ...helpers },
		children
	)
}

So if I have a <TextField /> like:

import React from 'react'
import TextField from '@material-ui/core/TextField'

export default (props) => {
	const { label, error, touched, field } = props

	return (
		<TextField
			label={label}
			error={touched && error != null}
			{...field}
		/>
	)
}

Then I can memoize it and hook it into Formik like:

<SmartField as={MyTextField} name="myField" label="My Field" />

I'm currently using this for all my components and it seems to work well, though I'm no React/Formik guru to know it's not falling into some nasty pitfall. Time will tell. I do like that I can use this to wrap all my input components, including more complex ones that I needed the useField hook for in the past, which makes it really straightforward to change stuff in my form.

Here's a demo sandbox I forked from @mayorandrew's.

@gilamran
Copy link

This issue is much more then just unwanted renders.
A bigger problem is when you use helpers in useEffect dependencies. If you do so, you'll get to an infinite loop of renders, as helpers reference changes on every render...

@0xR
Copy link

0xR commented Jul 26, 2020

I created a Typescript version of useFastField mentioned above. Might be helpful for you:

export function useFieldFast<Val>(
  propsOrFieldName: string | FieldHookConfig<Val>,
) {
  const [field, meta, helpers] = useField(propsOrFieldName);

  const actualHelpersRef = useRef<FieldHelperProps<Val>>(helpers);

  // On every render save newest helpers to actualHelpersRef
  actualHelpersRef.current.setValue = helpers.setValue;
  actualHelpersRef.current.setTouched = helpers.setTouched;
  actualHelpersRef.current.setError = helpers.setError;

  const consistentHelpersRef = useRef<FieldHelperProps<Val>>({
    setValue: (...args) => actualHelpersRef.current.setValue(...args),
    setTouched: (value: boolean, shouldValidate?: boolean) =>
      actualHelpersRef.current.setTouched(value, shouldValidate),
    setError: (...args) => actualHelpersRef.current.setError(...args),
  });

  return [field, meta, consistentHelpersRef.current] as const;
}

@gilamran
Copy link

gilamran commented Aug 3, 2020

@0xR You have a small typescript issue there with the generics, here's a fix:

export function useFieldFast<Val = any>(
  propsOrFieldName: string | FieldHookConfig<Val>,
) {
  const [field, meta, helpers] = useField<Val>(propsOrFieldName);

  const actualHelpersRef = useRef<FieldHelperProps<Val>>(helpers);

  // On every render save newest helpers to actualHelpersRef
  actualHelpersRef.current.setValue = helpers.setValue;
  actualHelpersRef.current.setTouched = helpers.setTouched;
  actualHelpersRef.current.setError = helpers.setError;

  const consistentHelpersRef = useRef<FieldHelperProps<Val>>({
    setValue: (...args) => actualHelpersRef.current.setValue(...args),
    setTouched: (value: boolean, shouldValidate?: boolean) =>
      actualHelpersRef.current.setTouched(value, shouldValidate),
    setError: (...args) => actualHelpersRef.current.setError(...args),
  });

  return [field, meta, consistentHelpersRef.current] as const;
}

@priley86
Copy link

priley86 commented Aug 14, 2020

Coming these implementations above in Typescript could also look like this:

/* eslint-disable @typescript-eslint/no-explicit-any */
import { FieldHelperProps, FieldHookConfig, FieldInputProps, useField } from 'formik';
import { useRef } from 'react';

export function useFieldFast<Val = any>(propsOrFieldName: string | FieldHookConfig<Val>) {
  const [field, meta, helpers] = useField<Val>(propsOrFieldName);

  const fieldRef = useRef<FieldInputProps<Val>>();
  fieldRef.current = field;

  const makeFieldShim = (field: FieldInputProps<Val>) => ({
    ...field,
    onBlur: (args: any) => fieldRef.current?.onBlur(args),
    onChange: (args: any) => fieldRef.current?.onChange(args),
  });

  const fieldShim = useRef(makeFieldShim(field));

  if (fieldShim.current.value !== field.value || fieldShim.current.name !== field.name) {
    fieldShim.current = makeFieldShim(field);
  }

  // On every render save newest helpers to actualHelpersRef
  const actualHelpersRef = useRef<FieldHelperProps<Val>>(helpers);
  actualHelpersRef.current.setValue = helpers.setValue;
  actualHelpersRef.current.setTouched = helpers.setTouched;
  actualHelpersRef.current.setError = helpers.setError;

  const consistentHelpersRef = useRef<FieldHelperProps<Val>>({
    setValue: (value: Val, shouldValidate?: boolean | undefined) =>
      actualHelpersRef.current.setValue(value, shouldValidate),
    setTouched: (value: boolean, shouldValidate?: boolean) =>
      actualHelpersRef.current.setTouched(value, shouldValidate),
    setError: (value: Val) => actualHelpersRef.current.setError(value),
  });

  return [fieldShim.current, meta, consistentHelpersRef.current] as const;
}

I am having some luck with this and wrapping the outer components w/ React.memo here. Once this issue is resolved, you simply replace useFieldFast references w/ useField I suppose. The React.memo optimizations can't hurt you.

@danieltott
Copy link

danieltott commented Aug 28, 2020

I ran into this today, and realized that the functions from useFormikContext do not suffer from this.

So, while this causes the signature of callback to change, which can result in unwanted effects like renders or firing of useEffect etc:

const [field, meta, { setValue, setTouched, setError }] = useField(name);

// callback is re-created every time any value changes in the form
const callback = useCallback(() => {
  setValue('x')
  setTouched(true)
  setError('y')
}, [setValue, setTouched, setError])

This will never re-set the value of callback during normal operation of a Formik form:

const [field, meta] = useField(name);
const { setFieldValue, setFieldTouched, setFieldError } = useFormikContext()

// callback is created only once and never re-creates
const callback = useCallback(() => {
  setFieldValue(name, 'x')
  setFieldTouched(name, true)
  setFieldError(name, 'y')
}, [setFieldValue, setFieldTouched, setFieldError])

You can even wrap that up into a new hook and use as a drop-in replacement:

const useFieldNew = (config) => {
  const [field, meta] = useField(config);
  const { setFieldTouched, setFieldValue, setFieldError } = useFormikContext();
  const helpers = useMemo(
    () => ({
      setValue: (...args) => setFieldValue(field.name, ...args),
      setTouched: (...args) => setFieldTouched(field.name, ...args),
      setError: (...args) => setFieldError(field.name, ...args)
    }),
    [setFieldTouched, setFieldValue, setFieldError, field.name]
  );

  return [field, meta, helpers];
};

Here's a sandbox illustrating: https://codesandbox.io/s/formik-use-global-helpers-hooks-example-2m9cy?file=/src/App.js

This may be a way forward for fixing up the original helpers in Formik - I haven't gotten that far yet.

Edited for clarity

@secodigo
Copy link

@danieltott, even so the component continues to render even if it is not changed

@danieltott
Copy link

@secodigo True, thank you. Edited my original comment to clarify the language.

@keithjgrant
Copy link

keithjgrant commented Sep 4, 2020

Desired Behavior

The helpers.setValue should only update when the value of the related form field got updated

This should be if the field definition changes, right, not the field value? A changed value doesn't mean the setter function needs to change (which could in fact invoke necessary useEffect/useCallback client code)

@abacaj
Copy link

abacaj commented Oct 25, 2021

This has been working for me without any max depth re-render issues (typescript code ahead):

function useFieldSafe<T>(fieldName: string): {
  setValue: (value: T) => void;
  setTouched: (touched: boolean) => void;
  error: string | undefined;
  value: T;
  isSubmitting: boolean;
} {
  const {
    setFieldValue,
    errors,
    values,
    setFieldTouched,
    registerField,
    isSubmitting,
  } = useFormikContext();

  useEffect(() => {
    registerField(fieldName, {});
  }, [fieldName, registerField]);

  return {
    setValue: useCallback(
      (value: T) => setFieldValue(fieldName, value),
      [fieldName, setFieldValue],
    ),
    setTouched: useCallback(
      (touched: boolean) => {
        setFieldTouched(fieldName, touched);
      },
      [setFieldTouched, fieldName],
    ),
    error: errors[fieldName as keyof typeof errors],
    value: (values as Record<string, unknown>)[fieldName] as T,
    isSubmitting,
  };
}

And to use:

const { isSubmitting, setTouched, setValue, error, value } = useFieldSafe<string>(name);

@johnrom
Copy link
Collaborator

johnrom commented Oct 25, 2021

This should be fixed in #3231

Names have been changed to useFieldProps, useFieldHelpers, and those use memoized values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests