eBay / nice-modal-react

A modal state manager for React.
https://ebay.github.io/nice-modal-react
MIT License
1.96k stars 110 forks source link

returned object from useModal hook have "unstable" identity #58

Closed mrudowski closed 9 months ago

mrudowski commented 2 years ago

Hi, so nice library!

and this fix for https://github.com/eBay/nice-modal-react/issues/53

just in time (I've just updated to 1.2.4) Thank you!

But could we do the same for whole object returned from useModal?

const userModal = useModal(UserInfoModal);

to make userModal referential equality all the time? so if I test it here like this:

const userModal = useModal(UserInfoModal);

React.useEffect(() => {
    console.log('endless show...');
    userModal.show();
}, [userModal]) // <- referential identity will change every time and we have endless loop here
mrudowski commented 2 years ago

after reading this discussion at https://github.com/tannerlinsley/react-query/issues/1858 and here https://github.com/tannerlinsley/react-query/issues/1905 I have to say it's not so simple... and maybe not quite in the right direction

after react-hook-form which return "stable" object from useForm hook I assumed that this is the way. But now I dunno

supnate commented 2 years ago

Right, this is something we can enhance but not that simple. For now, you can destruct the methods show, hide from userModal as dependencies to avoid loops.

janczizikow commented 2 years ago

Just adding my two cents 😄

There are many ways to avoid the infinite re-rendering, one of which was already mentioned (destructuring).

Other options:

Pass reference to a callback in useEffect (without destructuring):

const userModal = useModal(UserInfoModal);

React.useEffect(() => {
    console.log('endless show...');
    userModal.show();
}, [userModal.show]) // <-- pass userModal.show

Pass whole object, but call the callback conditionally:

const userModal = useModal(UserInfoModal);

React.useEffect(() => {
    if (!userModal.visible) { // <-- just as an example
      console.log('endless show...');
      userModal.show();
    }
}, [userModal])

As for memoizing the return value of useModal - as you mentioned it's probably not so easy. One way that I could think of could be some kind shallow equality checks of userModal.args either in the reducer in the useModal hook 🤔 But I think that will add more complexity to the lib.

mrudowski commented 2 years ago

Yes, of course,

but after using react-hook-form I started to question myself - what is right approach for it in react/hook world.

Because solution itself is rather easy, like here for example: https://github.com/react-hook-form/react-hook-form/blob/7c5009f5280f8192e71b39dae564e2e473701150/src/useForm.ts

// create ref
const _formControl = React.useRef<
    UseFormReturn<TFieldValues, TContext> | undefined
  >();

// update it's attributes
_formControl.current.formState = ...

// and finally return it
return _formControl.current;

But when I think more about it it's rather not as common as I thought. For example React useState will always returns new array so we always are destructuring the array to get "stable" state variable and setState method

const [ourState, setOurState] = useState();
jilherme commented 1 year ago

@mrudowski did you manage to make react-hook-form work with modals in react-nice-modal?

mrudowski commented 1 year ago

@mrudowski did you manage to make react-hook-form work with modals in react-nice-modal?

I use it all the time without any problems. I was only wondering about returning object from any hook and the right approach to it in react/hook world.

supnate commented 9 months ago

should be resolved in v1.2.11.

nik32 commented 5 months ago

should be resolved in v1.2.11.

@supnate does this mean that from v1.2.11... we can use the userModal inside of the effect dependencies array without having to worry about infinite re-renders? OR we would still need to destruct the methods show, hide from userModal as dependencies to avoid loops.

xeinebiu commented 5 months ago

@janczizikow

` const userModal = useModal(UserInfoModal);

React.useEffect(() => { console.log('endless show...'); userModal.show(); }, [userModal.show]) // <-- pass userModal.show `

This wont work with prettier nor eslint. They do not allow to pass functions due to "this" binding.

Even destructuring the show function, it stills ends up on infinite loop. I think this library should use internally useCallbacks so it returns the same functions when nothing really changes, otherwise on this Issue only workarounds are proposed.

Version 1.2.23 and the loop still exists.