Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix] Prevent outside click events for unmounted component #26

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BaZzz01010101
Copy link

I have two nested components, each wrapped into own OutsideClickHandler. Both components is unmouning on outside clicks by calling a handler function. This handler functions are located in their parents and passed to the components in props. Handler functions are setting this.state which in turn unmount the child (our component, wrapped into OutsideClickHandler).

The problem appears when user make click out of the outer component of this pair. In this case the click event is emitting for both OutsideClickHandler components but the order of called handlers is:

  1. outer/parent component
  2. inner/child component

And therefore the inner/child component receive the outside click event after outer/parent component unmounted. In turn it's also trying to set state to unmount itself. At this moment the ReactJs show his warning:

Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.

I see that current implementation removes event listeners in the componentWillUnmount but this is not enough in the case of nested components, because event is already in queue and will call the handler anyway even after component is unmouned.

@BaZzz01010101
Copy link
Author

BaZzz01010101 commented Mar 25, 2019

P.S. I used _isMounted because components still contains deprecated read-only isMounted property which always undefined.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a regression test?

@BaZzz01010101
Copy link
Author

BaZzz01010101 commented Jun 12, 2019

I can try, but I have no experience with test frameworks.

Update:
I managed to run test by npm run tests-only.
There is a problem with test because now the event fires only if component is mounted. But the test simulates mouseup event on not mounted instance. Should I just add
instance._isMounted = true;
before
instance.onMouseUp(event);
to simulate mounting of the component?

describe('when `this.childNode` does not contain `e.target`', () => {
it('is a noop if `this._isMounted` is `false`', () => {
const spy = sinon.spy();
const wrapper = shallow(<OutsideClickHandler onOutsideClick={spy} />);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use mount here, and then wrapper.unmount() to test unmounting a component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants