Pomax / react-onclickoutside

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

Use empty object instead of null as default handlerOptions #356

Closed egudkov closed 1 year ago

egudkov commented 3 years ago

Hi!

Motivation

Our team found a conflict between the code of react-onclickoutside and the code of some wordpress plugins and themes. Those plugins use code that makes event listeners passive by default in order to improve scrolling experience, see example by the link - https://stackoverflow.com/a/45974787

This code overwrites addEventListener and according to the following lines...

    EventTarget.prototype.addEventListener = function(type, listener, options) {
      var usesListenerOptions = typeof options === 'object';
      var useCapture = usesListenerOptions ? options.capture : options;

...there will be an error if null is passed as options, because typeof null === 'object' is true and then null.capture leads to a TypeError: Cannot read property 'capture' of null error.

And currently there is the case when react-onclickoutside passes null as options to addEventListener. So it leads to the error when our product (with react-onclickoutside) is embedded to the user site that has the wordpress plugin.

Changes

The proposal is to use empty object as default options value instead of null. It looks like more expected behaviour according to specs and it fixes the conflict.

Pomax commented 3 years ago

I'd like to point out that overriding the addEventListener function in the way you're showing is pretty much its own bug right there, and should be reported and fixed. Adding a null test to that code should be trivial and will fix things for every library by fixing the problem, rather than by "fixing" already fine code.

    EventTarget.prototype.addEventListener = function(type, listener, options) {
      var usesListenerOptions = options ? typeof options === 'object' : false;
      var useCapture = usesListenerOptions ? options.capture : options;

Done.

egudkov commented 3 years ago

I agree 100% - there should be check for null. The problem is that there are at least two plugins with this bug, maybe more. I will contact the authors and ask to add a null test but I'm not sure that the bug will not come back later with other plugins. So, the idea is to also make a change in this lib to prevent new cases of that bug.

Anyway, we are going to delete the component where we used react-onclickoutside lib (not because of the described problem) and this case will not be an issue for us. So, I guess it is up to you to decide what to do with this pull request.

Pomax commented 3 years ago

Cheers. If you can drop a link to the issue you filed with them once you file it, I will probably merge this in. (but not before, because the most important thing is that they fix it. Until they do, me fixing this lib's code just encourages them to not bother fixing their code).