mantine icon indicating copy to clipboard operation
mantine copied to clipboard

[@mantine/form] use-form: Make functions referentially stable

Open mattrunyon opened this issue 2 years ago • 5 comments

Fixes #1383

The issue before was when getInputProps was called, valuesRef hadn't been updated by useEffect yet (guessing React was deferring this effect). This caused the value and onChange to have an alternating state where typing "abcd" would result in "ac" and "bd" shown.

Now the ref should always be up to date when useForm is called so any calls to getInputProps shouldn't be able to occur with stale values.

The changes from the previous PR are in the 2 new hooks

Tested using all the Storybook examples

mattrunyon avatar May 10 '22 05:05 mattrunyon

@mattrunyon did you have a chance to test it with React 18?

rtivital avatar May 14 '22 11:05 rtivital

Tested on React 18 (but not with any explicit concurrent features as I haven't used them before). With useLayoutEffect the same bug appears (in 17 and 18). I'll just make getInputProps and getListInputProps depend on values and errors so they're still optimized some, but not completely stable functions.

useLayoutEffect happens after DOM mutation and before paint. useEffect happens after mutation and paint. So both have the issue because getInputProps is called before/during DOM mutation on render.

I'm not entirely sure if there would be issues not using useEffect to update the ref in React 18 (again, I haven't used concurrent features), but my guess is in a concurrent render React might use the same ref object which could create issues if the concurrent render is discarded. Not entirely sure though since the values for the refs should only change on user inputs, so any renders which change the ref should be actual renders and not concurrent.

I also tested by adding this to Form.demo.password since it was the original reason I wanted the functions to be stable. The use case was update form validation on user input

useEffect(() => {
    form.validate();
  }, [form.values]);

mattrunyon avatar May 14 '22 19:05 mattrunyon

Well, seems very complicated, maybe there is a way to remove refs and implement it with state only

rtivital avatar May 14 '22 20:05 rtivital

Implementing with state you will have to recreate most functions each render because any time the values change, any function which uses the values would change. Nearly every function uses the state at some point, so useCallback would be useless then since they'll change on almost every call.

Or you have to have all the functions take in values, like form.setFieldValue(values, fieldName, value) which is annoying

mattrunyon avatar May 14 '22 20:05 mattrunyon

@mattrunyon I've updated v5 branch to react 18, it turns out if you just update react in package.json, storybook will not use it and turn off new async features by default. Can you check if your changes work with the updated v5 branch? Also, can you please resolve conflicts?

rtivital avatar May 18 '22 14:05 rtivital