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 memory leak with regression test #25

Merged
merged 2 commits into from Jun 12, 2019

Conversation

Projects
None yet
3 participants
@Superoryco
Copy link
Contributor

commented Mar 21, 2019

Fix memory leak with regression test
#20

tranvansang and others added some commits Dec 17, 2018

@Superoryco Superoryco closed this Mar 21, 2019

@Superoryco Superoryco reopened this Mar 21, 2019

@Superoryco Superoryco closed this Mar 21, 2019

@Superoryco Superoryco reopened this Mar 21, 2019

@ljharb

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

In the future, please do not open up duplicate PRs - post a link to your commit and I can add it to the original PR.

@Superoryco

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

In the future, please do not open up duplicate PRs - post a link to your commit and I can add it to the original PR.

Sorry about that...

It looks like jsdom has some environment conflicts with this repository, and I think it is necessary to be included during the test. How can I make it pass? @ljharb

Superoryco@b68c551

@ljharb

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

I'm not clear on why it's needed.

@Superoryco

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

I'm not clear on why it's needed.

In 1.2.3

If we mousedown on outside area without mouseup serval times, multiple zombie mouseup eventListener will be added to document. In next click cycle(mousedown then mouseup), these zombies will trigger props.onOutsideClick at the same time which should be an unexpected behavior.

#20 fixed the problem. To make a regression test case, we need to trigger mouseup eventListener in document instead of call instance.onMouseUp(event);(This will call props.onOutsideClick only once but still leaves zombies unexposed).

  it('calls onOutsideClick only once', () => {
    const spy = sinon.spy();
    const wrapper = mount(<OutsideClickHandler onOutsideClick={spy} />);
    const instance = wrapper.instance();

    instance.childNode = {};
    expect(contains(instance.childNode, target)).to.equal(false);

    instance.onMouseDown(event);
    instance.onMouseDown(event);
    instance.onMouseDown(event);
    
    const mouseUpEvent = new Event('mouseup');
    document.dispatchEvent(mouseUpEvent);

    expect(spy).to.have.property('callCount', 1);
  });

document.dispatchEvent(mouseUpEvent) requires jsdom and so does onMouseDown function.
That's why I think jsdom is needed...

Any suggestions...? @ljharb

@ljharb

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

Instead of adding jsdom, can we mock document.dispatchEvent, or, just manually invoke prop functions to simulate what that does?

@Superoryco

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

@ljharb Great...
But I have no idea how to make a fake document.dispatchEvent which may trigger event listeners.
How about this:
Lets focus on the number of event listeners...
I made a document object with addEventListener & removeEventListener spy functions to check the count, is it a good approach?

@Superoryco

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

By the way, why skip lifecycle methods? describe.skip('lifecycle methods', () => {

@ljharb ljharb force-pushed the Superoryco:master branch from acdf72f to 03230cc Jun 12, 2019

@ljharb

ljharb approved these changes Jun 12, 2019

Copy link
Member

left a comment

Rebased on top of #20; thanks.

@ljharb ljharb merged commit 03230cc into airbnb:master Jun 12, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

ljharb added a commit that referenced this pull request Jun 12, 2019

[Fix] prevent memory leak if `mousedown` is fired, but `mouseup` isn’t
Merge pull request #20 from tranvansang/patch-1, #25 from Superoryco/master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.