Pomax / react-onclickoutside

An onClickOutside wrapper for React components
MIT License
1.83k stars 187 forks source link

Functional component docs issue #351

Closed penx closed 3 years ago

penx commented 3 years ago

The readme suggests to do the following:

https://github.com/Pomax/react-onclickoutside/blob/41ce5a6a6872fd1847ad6fd7f3d0b7412fd236b3/README.md#L83-L99

Menu.handleClickOutside is appending the handler to component definition rather than to the instance of the component, meaning this can introduce buggy side effects, e.g. if you were to have 2 instances of Menu then the handler would call twice for the second instance but not at all for the first instance.

This can be tested on the code sandbox linked in the docs https://codesandbox.io/s/vn66kq7mml

by:

Pomax commented 3 years ago

There is no "component instance" when you're using a function, though. It's not a class/prototyped object. Instead, "the function gets run in several places". If you need to run your function multiple times in different places that all stay active, then yes, you can't "save" the handling as a function property.

penx commented 3 years ago

Is there any way to use react-onclickoutside on functional components that have multiple instances? If so, it would be good to have that approach in the docs.

I expect this would be a common use case, for example a custom select component. If there's not an alternative (or if this example code was to remain in place for other reasons), I think it would be useful to add a caveat to the example in the docs, as I don't think this restriction would be immediately obvious to everyone.

penx commented 3 years ago

There is no "component instance"

understood, it's just the best wording I could think of to describe the issue :)

Pomax commented 3 years ago

Not really, no: that's kind of the point of functions, there flat out are no "components" to work with. the function runs and spits out an anonymous virtual DOM element. So if you need a single one: this will work. If you need more than one: there's nowhere to "pin" the functions that the HoC code needs to run in a way that those are tied to that specific instance.

I'd love to take the time to write a proper useEffect hook for it, so that folks who use functions can get the same functionality, but unless someone steps forward to help fund the work this library needs, my other commitments prevent me from dedicating the time required to implement that.

moloko commented 3 years ago

unless someone steps forward to help fund the work this library needs, my other commitments prevent me from dedicating the time required to implement that.

Completely understandable... but in the meantime is there any chance of updating the 'Functional Component with UseState Hook' section of the README to warn people about this limitation?

Pomax commented 3 years ago

updated the docs, modern React basically doesn't need this HoC, it's trivial to implement with useRef and useEffect.

https://github.com/Pomax/react-onclickoutside/blob/master/README.md#functional-component-with-usestate-hook