removing args as a return value of useModal hook

[janczizikow]

Hey I realized that issue described in #53 is still present when calling the show handler with arguments 😞

The following works as desired after the fix:

const [counter, setCounter] = React.useState(0);
const userModal = useModal(UserInfoModal);
const { show } = userModal; 

React.useEffect(() => {
  if (counter >= 2) {
    show();
  }
}, [counter, show]) 

The effect doesn't cause re-rendering loop as it was before. But if we call show with a non-primitive type it would again cause an infinite re-rendering loop 😢

const [counter, setCounter] = React.useState(0);
const userModal = useModal(UserInfoModal);
const { show } = userModal; 

React.useEffect(() => {
  if (counter >= 2) {
    show({ title: 'test' }); // <- passing some args to `show`
  }
}, [counter, show]) // <- show identity will change every time we call show with args

It's the same reason for it as described in #54. This time though the problem is modalInfo.args - it's an object and it will change on every render. I think there are 2 possible ways of solving this:

Option 1: Don't memoize args returned from useModal instead memoize just callbacks:

export function useModal(modal?: any, args?: any): any {
// ...
-  return React.useMemo(
-   () => ({
-     id: mid,
-     args: modalInfo?.args,
-    visible: !!modalInfo?.visible,
-    keepMounted: !!modalInfo?.keepMounted,
-     show: (args?: Record<string, unknown>) => show(mid, args),
-     hide: () => hide(mid),
-     remove: () => remove(mid),
-     resolve: (args?: unknown) => {
-      modalCallbacks[mid]?.resolve(args);
-       delete modalCallbacks[mid];
-    },
-    reject: (args?: unknown) => {
-      modalCallbacks[mid]?.reject(args);
-      delete modalCallbacks[mid];
-    },
-    resolveHide: (args?: unknown) => {
-      hideModalCallbacks[mid]?.resolve(args);
-      delete hideModalCallbacks[mid];
-    },
-    }),
-    [mid, modalInfo?.args, modalInfo?.keepMounted, modalInfo?.visible]
- );
+  const showCallback = React.useCallback(
+    (args?: Record<string, unknown>) => show(mid, args),
+    [mid]
+  );
+  const hideCallback = React.useCallback(() => hide(mid), [mid]);
+  const removeCallback = React.useCallback(() => remove(mid), [mid]);
+  const resolveCallback = React.useCallback(
+    (args?: unknown) => {
+      modalCallbacks[mid]?.resolve(args);
+      delete modalCallbacks[mid];
+    },
+    [mid]
+  );
+  const rejectCallback = React.useCallback(
+    (args?: unknown) => {
+      modalCallbacks[mid]?.reject(args);
+      delete modalCallbacks[mid];
+    },
+    [mid]
+  );
+  const resolveHide = React.useCallback(
+    (args?: unknown) => {
+      hideModalCallbacks[mid]?.resolve(args);
+      delete hideModalCallbacks[mid];
+    },
+    [mid]
+  );

+ return {
+    id: mid,
+    args: modalInfo?.args,
+    visible: !!modalInfo?.visible,
+    keepMounted: !!modalInfo?.keepMounted,
+    show: showCallback,
+    hide: hideModalCallbacks,
+    remove: removeCallback,
+    resolve: resolveCallback,
+    reject: rejectCallback,
+    resolveHide,
+  };
}

Now we can rely on show, because they will only change if id of modal changes, which I don't think it's a common case. All is well and if we desctructure show from the return value it works in our example:

const [counter, setCounter] = React.useState(0);
const userModal = useModal(UserInfoModal);
const { show } = userModal; // we have to separate show 

React.useEffect(() => {
  if (counter >= 2) {
    show({ title: 'test' }); // <- passing some args to `show`
  }
}, [counter, show]) // <- show identity only changes when id of modal changes, so this is 👌 

// this is also fine:
React.useEffect(() => {
  if (counter >= 2) {
    userModal.show({ title: 'test' }); // <- passing some args to `show`
  }
}, [counter, userModal.show]) // <- show identity only changes when id of modal changes, so this is 👌 

But if someone passes the whole object returned returned from the hook it will again trigger an re-rendering loop

const [counter, setCounter] = React.useState(0);
const userModal = useModal(UserInfoModal);

React.useEffect(() => {
  if (counter >= 2) {
    userModal.show({ title: 'test' }); // <- passing some args to `show`
  }
}, [counter, userModal]) // <- we pass the whole state of userModal which is gonna change `modalInfo.args` in the reducer indefinitely :(

Option 2: Don't return args in useModal

  return React.useMemo(
    () => ({
      id: mid,
-    args: modalInfo?.args,
      visible: !!modalInfo?.visible,
      keepMounted: !!modalInfo?.keepMounted,
      show: (args?: Record<string, unknown>) => show(mid, args),
      hide: () => hide(mid),
      remove: () => remove(mid),
      resolve: (args?: unknown) => {
        modalCallbacks[mid]?.resolve(args);
        delete modalCallbacks[mid];
      },
      reject: (args?: unknown) => {
        modalCallbacks[mid]?.reject(args);
        delete modalCallbacks[mid];
      },
      resolveHide: (args?: unknown) => {
        hideModalCallbacks[mid]?.resolve(args);
        delete hideModalCallbacks[mid];
      },
    }),
-   [mid, modalInfo?.args, modalInfo?.keepMounted, modalInfo?.visible]
+   [mid, modalInfo?.keepMounted, modalInfo?.visible]
  );

This eliminates the problem of comparing the modalInfo.args with === and we can pass return values from useModal hook however we like in useEffect or other hooks. I think it's probably a better approach in my opinion, but it also means a change in the API.

I would be curious to hear your opinion and discuss whether removing args from the return value would be something you would consider? Would be happy to open PR for it as well if we decide which approach we would like to take 🙂

[supnate]

Hey @janczizikow , thanks for the deep dive into the code! It's a valid problem for reducing unnecessary renderings.

Option 1 looks good to me. It provides opportunity for a developer to optimize the rendering. From immutability perspective a modal handler depends on some state(args) it uses seems reasonable. So we can just optimize the methods on the handler.

Option 2 will break the functionality because the args from modal handler is used in the NiceModal.create method in HOC. So we can't remove the args property from the return value of useModal.

However, this problem is similar with which is described at: https://stackoverflow.com/questions/55724642/react-useeffect-hook-when-only-one-of-the-effects-deps-changes-but-not-the-oth . Actually we only want to do something when part of dependences changed. So manual control is a suggested approach (check if counter is actually changed).

[janczizikow]

Hey @supnate thanks for your answer! I didn't realize args are used in NiceModal.create! Totally makes sense to have it as a return value in the hook. Now I get it 😄

The problem with counter is slightly made up. Perhaps a better example (more real) would be to show a modal depending on URL query. Often it's also the case that we re-use existing callback and use it in multiple places (onClick + useEffect):

const { show } = useModal(UserInfoModal);
const [users, setUsers] = useState<User[]>(mockData);
const handleNewUser = useCallback(() => {
  show({ title: 'New User' }).then((newUser) => {
    setUsers([newUser, ...users]);
    // navigate to a new page without ?action=show in the URL
  });
}, [show, users]);

useEffect(() => {
  if (window.location.search.includes("action=show")) {
    handleNewUser();
  }
}, [handleNewUser]);

return (
// ...
      <Button type="primary" onClick={handleNewUser}>
        + New User
      </Button>
// ...
)

Ofc we could skip passing [handleNewUser] in useEffect, but it will trigger a warning from eslint if using react-hooks and it's generally not recommended in react docs. Somehow I think it feels more natural to pass the dependency.

It could still be a nice improvement to memoize just the callbacks as described in option 1. I'll open a PR with the changes for your consideration 🙂

EDIT: another "fix" to prevent constant re-rendering could be to also to check if the modal is not open. Which should also work with the current useMemo

useEffect(() => {
  if (!modal.visible && window.location.search.includes("action=show")) {
    handleNewUser();
  }
}, [modal.visible, handleNewUser]);