[1.x] Fix useForm re-renders by memoizing functions in React

[Hasan-Mir]

When using the useForm() in a React component that has some heavy pure components(components using React.memo) as children and passing some methods returned by useForm() like setData, clearErrors, etc to them, on every re-render, these methods returned by useForm() are getting new references so new props are passed to these React.memo()ed children and they will be re-rendered unnecessarily.
This pull request fixes this issue by using useCallback for those methods in the useForm hook.

[jameshulse]

I just want to flag that this PR (mostly) fixed some issues that we were running into. I have ended up taking a copy of this useForm implementation and referencing it directly in our application. So just giving a +1 that this PR could be prioritised to bring in.

I also needed to add data as a dependency of the transformFunction callback to ensure that it worked correctly for how we are using the hook:

  const transformFunction = useCallback(
    (callback) => {
      transform = callback;
    },
    // This dependency was added
    [data]
  );

[Hasan-Mir]

Hi @jameshulse, thank you for the hint, I've fixed it in the latest commit by using the useRef instead of declaring the transform as let transform = (data) => data. This way the transform function maintains the same reference between re-renders.

Furthermore, using useRef addresses another issue in the current useForm implementation. If we use a state inside of the callback that we pass to the transform function and we update that state without updating data (calling setData), the transform function will not utilize the updated state. Instead, it will use the stale version of it.

function GalleryForm(props) {
  const { data, submit, errors, setData, processing, transform } = useForm({
    picture: null,
    picurl: gallery?.type === 0 ? gallery?.url : "",
  });

  const [pictureSourceType, setPictureSourceType] = useState(
    data.picurl ? "pic_url" : "pic_upload"
  );

  transform((prevData) => ({
    ...prevData,
    picture: prevData.type === 0 || pictureSourceType === "pic_url" ? null : prevData.picture,
    picurl: prevData.type === 1 || pictureSourceType === "pic_upload" ? null : prevData.picurl,
  }));

  function handelSubmit(e) {
    e.preventDefault();
    submit("post", "gallery", {
      forceFormData: true,
    });
  }

  return (
    <form>
      {/* ... */}

      <TabButton
        onClick={() => {
          setPictureSourceType("pic_upload");
        }}
      >
        upload image
      </TabButton>
      <TabButton
        onClick={() => {
          setPictureSourceType("pic_url");
        }}
      >
        use image url
      </TabButton>

      {/* ... */}
    </form>
  );
}

Here when I update the pictureSourceType (for example from "pic_url" to "pic_upload") and then try to submit the form, the value of pictureSourceType remains as "pic_url" when the transform function executes!
This is due to the dependency list of the submit function, which is [data, setErrors]. As long as the data remains unchanged, the submit function maintains the same function reference from the last re-render where data was modified and received a new reference. So the value of transform function belongs to that render.
To address this issue without using useRef we need to call setData wherever we update that state that we use inside of the transform's callback (note that I don't want to set picture or picurl to null when I change the pictureSourceType tab). This approach ensures that the submit function employs the most recent value of the transform.

function GalleryForm(props) {
 // ...

 return (
   <form>
     {/* ... */}

     <TabButton
       onClick={() => {
         setPictureSourceType("pic_upload");
+        setData((prevData) => ({ ...prevData })); 
       }}
     >
       upload image
     </TabButton>
     <TabButton
       onClick={() => {
         setPictureSourceType("pic_url");
+        setData((prevData) => ({ ...prevData })); 
       }}
     >
       use image url
     </TabButton>

     {/* ... */}
   </form>
 );
}

[kendepelchin]

Anyone has an idea on what this PR is waiting? Or could I patch package this somehow?

[derrickreimer]

These changes seem good & safe. It requires a bit of diligence to keep the dependency arrays up-to-date on the useCallback calls, but I think worthwhile for performance.

[Hasan-Mir]

@reinink It has been 431 days since the last commit!😶

[pedroborges]

👍

[joetannenbaum]

This looks good, thank you! As @derrickreimer mentioned, we just have to take care to maintain the dependency arrays.

[Hasan-Mir]

@joetannenbaum
Adding Eslint with eslint-plugin-react-hooks simplifies maintaining the dependency arrays.