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

Form hook causes infinite re-render when using as required effect dependency #5338

Closed
lukasbash opened this issue Nov 27, 2023 · 15 comments
Closed

Comments

@lukasbash
Copy link

What package has an issue?

@mantine/form

Describe the bug

In the example of your docs (https://mantine.dev/form/recipes/#set-initial-values-with-async-request) you use the useEffect as a mount-only cycle.
However, considering that I can have the form mounted and the actual values can change via props (or any other hook), I might want to extend the useEffect hook with other properties that change.

Consider I want to change the values with an effect like this:

useEffect(() => {
  const values = {
    a: prop_a,
    b: prop_b,
  };

  form.setValues(values);
  form.resetDirty(values);
}, [prop_a, prop_b]);
// React Hook useEffect has a missing dependency: 'form'. Either include it or remove the dependency array.

With the example above, eslint complains about the form object not being a dependency (which is technically correct, isn't it?). Now if I add the form object as a dependency (regardless of either using form directly or destructuring setValues and resetDirty), I get an infinite render cycle:

image

How do you explain the correct approach, re-rendering on single prop changes and update the values accordingly while still being linting-compatible? Our project follows pretty strict linting rules, so exclusions are not really possible here. Also I assume re-setting initial values as a "one-time-re-render" is not an option. Further I guess that the form relies on all components being controlled and therefore always re-renders on a change, whereas a setValue call is a reason here.

Please give me some input on the technical correctness, and a working, linted code 🙏

What version of @mantine/* packages do you have in package.json? (Note that all @mantine/* packages must have the same version in order to work correctly)

7.1.5

If possible, please include a link to a codesandbox with a minimal reproduction

No response

Do you know how to fix the issue

No

Do you want to send a pull request with a fix?

No

Possible fix

No response

@rtivital
Copy link
Member

rtivital commented Dec 3, 2023

Do not add form as a dependency in useEffect – there is no reason to do so. If you really need this feature, you can wrap form in useMemo, but it does not really make any sense.

@rtivital rtivital closed this as completed Dec 3, 2023
@lukasbash
Copy link
Author

Do not add form as a dependency in useEffect – there is no reason to do so. If you really need this feature, you can wrap form in useMemo, but it does not really make any sense.

While this the most obvious option, I am still confused why it does not make sense. As far as I understand the official documentation, every reactive value needs to be a declared dependency.

But useMemo is worth a try, thanks a lot!

@twiddler
Copy link
Contributor

twiddler commented Feb 4, 2024

@rtivital Why does useForm (need to) change with every render? useForm already uses useCallback excessively, so this seems like unnecessary overhead, doesn't it?

Actually, useForm returns some properties that--at first glance--should probably be memo'ed, too:

  • resetDirty
  • getInputProps
  • onSubmit
  • getTransformedValues
  • isDirty

What do you think about breaking down the result into 1) the handlers, which should (almost) never change, and 2) the values, which should change often? Then we could do something like:

function useForm() {
    const [values, setValues] = useState({})

    const isDirty = useMemo(() => Object.keys(values).length > 0, [values])

    const reset = useCallback(function () {
        setValues({})
    }, [])

    const handlers = useMemo(
        () => ({
            setValues,
            reset,
        }),
        [reset]
    )

    const observables = useMemo(
        () => ({
            values,
            isDirty,
        }),
        [values, isDirty]
    )

    return useMemo(() => [handlers, observables] as const, [handlers, observables]) 
}

function Foo() {
    const [formHandlers, formObservables] = useForm()

    useEffect(
        function () {
            // Only runs when the handlers change (which is probably never)
        },
        [formHandlers]
    )

    useEffect(
        function () {
            // Only runs when the values change (which is probably often)
        },
        [formObservables]
    )

    

    return (
        <>
            <Button
                onClick={function() {
                    formHandlers.setValues({ a: Math.random() })
                }}
            >
                Change values
            </Button>

            <pre>{JSON.stringify(formObservables)}</pre>
        </>
    )
}

This is essentially the same as writing { initialize, ...form } (and so on) which also solve the dependency array issues, but it's less verbose.

Also, those who do not care about destructuring and just write const form = useForm() would not receive a new form when something else in the component triggers a re-render.

Appreciate your feedback. 🙏

@twiddler
Copy link
Contributor

@rtivital How'd you feel about wrapping some more things in useCallbacks? I'd be happy to give it a shot myself.

@rtivital
Copy link
Member

All form handlers are already wrapped in useCallback

@twiddler
Copy link
Contributor

twiddler commented Mar 3, 2024

Nope, see e.g. getInputProps.

@ronaldaraujo
Copy link

I'm having the same problem here. I've already tried using useMemo, but the page still keeps rerendering.

In the local development environment I don't feel the slowness, but in the production environment there is a very large delay. Which ends up bothering the user when filling out the fields.

In the video below, I even removed useEffect. However, the rerender continues 😞

vokoscreen-2024-03-27_09-36-42.mp4
const form = useForm<FormValues>({
  initialValues: {
    name: '',
    email: '',
    zip_code: '',
    state: '',
    city: '',
    address: '',
    number: '',
    neighborhood: '',
    complement: '',
    latitude: '',
    longitude: ''
   },
   validate: yupResolver(schema)
});

...

<Box
  component="form"
  onSubmit={form.onSubmit(onSubmit)}
  className="space-y-2"
>
...
<TextInput
  label="Nome"
  placeholder="Nome"
  {...form.getInputProps('name')}
/>

Any solution?

@rtivital
Copy link
Member

use-form stores values in state, it is expected behavior to rerender on each field change. If your app works correctly in development but worse in production, it is most likely not related to the form updating state. React heavily optimizes state changes in prod which results in exactly opposite – the same code works much faster in production. You can use a performance tab to find the code that causes the issue.

@ronaldaraujo
Copy link

Thank you for the response, @rtivital. In fact, I think Mantine is an incredible job 👏

But there is definitely something strange causing this rerender. For example, just for testing purposes, I used the useForm from react-hook-form and didn't get this amount of renderings (see the video below). Performance also improved in the production environment.

vokoscreen-2024-03-27_14-13-20.mp4

Anyway...

I'm not sure if it's a problem with my application, but I haven't figured out what it could be yet. For now, I'll use react-hook-form because in a production environment the improvement is noticeable.

Thanks!

@Gothfrid
Copy link

Gothfrid commented Jun 4, 2024

you can try smth like this

const { setValues, ...form } = useForm({ initialValue })

useEffect(() => {
  setValues(newData)
}, [newData, setValues])

@ronaldaraujo
Copy link

you can try smth like this

const { setValues, ...form } = useForm({ initialValue })

useEffect(() => {
  setValues(newData)
}, [newData, setValues])

I tried! But I got the same result :(

@nielsVoogt
Copy link

nielsVoogt commented Jun 12, 2024

I'm having the same problem here. I've already tried using useMemo, but the page still keeps rerendering.

In the local development environment I don't feel the slowness, but in the production environment there is a very large delay. Which ends up bothering the user when filling out the fields.

In the video below, I even removed useEffect. However, the rerender continues 😞

Any solution?

I also ran into this issue just now and noticed that the validate seemed to be responsible for this behaviour. Adding mode: 'uncontrolled' to the useForm hook fixed it for me. Try this:

const form = useForm<FormValues>({
  mode: 'uncontrolled'
  initialValues: {
    name: '',
    ...
  },
  validate: yupResolver(schema)
 });

@ronaldaraujo
Copy link

ronaldaraujo commented Jun 12, 2024

I'm having the same problem here. I've already tried using useMemo, but the page still keeps rerendering.
In the local development environment I don't feel the slowness, but in the production environment there is a very large delay. Which ends up bothering the user when filling out the fields.
In the video below, I even removed useEffect. However, the rerender continues 😞
Any solution?

I also ran into this issue just now and noticed that the validate seemed to be responsible for this behaviour. Adding mode: 'uncontrolled' to the useForm hook fixed it for me. Try this:

const form = useForm<FormValues>({
  mode: 'uncontrolled'
  initialValues: {
    name: '',
    ...
  },
  validate: yupResolver(schema)
 });

Thank you for answer, but what's your version of the Mantine? Here I receive this error

Object literal may only specify known properties, and 'mode' does not exist in type 'UseFormInput<FinanceProps, (values: FinanceProps) => FinanceProps>'.ts(2353)

I use version ^7.5.3.

@ronaldaraujo
Copy link

@nielsVoogt update my version to 7.10.1 and it worked. Actually, more or less. See:

vokoscreen-2024-06-12_10-38-11.mp4

My description field has stopped causing the rerender, but my value field still causes it. But it was a big step, thank you.

@songkeys
Copy link
Contributor

I’m not sure if it’s an issue with React, but I often find myself struggling with the "exhaustive-deps" problem.

Based on this demo for loading initial data (and many others):

useEffect(() => {
loadInitialValues().then((values) => {
form.setValues(values);
form.resetDirty(values);
});
}, []);

The common practice is to avoid including form in the useEffect. If you do include it, you’ll end up with infinite re-renders.

If you also use ESLint with the "react-hooks/exhaustive-deps" rule, you’ll encounter this error:

React Hook useEffect has a missing dependency: 'form'. Either include it or remove the dependency array. eslintreact-hooks/exhaustive-deps

As a result, developers often need to disable this line in the linting:

   useEffect(() => { 
     loadInitialValues().then((values) => { 
       form.setValues(values); 
       form.resetDirty(values); 
     });
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, []);

This is typically what I do because I know it’s safe.

However, with the introduction of "react-compiler" recently, if you also use ESLint with "react-compiler/react-compiler", you’ll see this error:

React Compiler has skipped optimizing this component because one or more React ESLint rules were disabled. React Compiler only works when your components follow all the rules of React; disabling them may lead to unexpected or incorrect behavior. eslint(react-compiler/react-compiler)

Basically it means you won't benefit from react-compiler in this entrie component because you have disabled a React ESLint.

Is this expected? I don't know. But maybe I should turn off these linters.

Is the usage of @mantine/hooks incorrect? I don't know. But maybe -

  • For example, when listening for changes and setting form values, we should use mode: "uncontrolled" and form.watch, as demonstrated here:

const form = useForm({
mode: 'uncontrolled',
initialValues: {
name: '',
email: '',
},
});
form.watch('name', ({ previousValue, value, touched, dirty }) => {
console.log({ previousValue, value, touched, dirty });
});

  • For another example, when loading initial data, we should provide an API similar to what’s done in react-hook-form:

https://github.com/react-hook-form/documentation/blob/5815afc8b89be448c7a4c506e950f2561ee98f47/src/data/api.tsx#L187-L190

Or perhaps it’s just the tricky UX patterns in React that leave every developer feeling puzzled from time to time.

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

No branches or pull requests

7 participants