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

Clear renderCalendarInfo timeout before assigning a new timeout (#1323) #2021

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

Conversation

mackinleysmith
Copy link

This is a do-over of @mstanaland's work on #1736 to fix a memory leak issue where the timeout is not cleared before being set again. I am doing this so that we can be current with master, since the original PR went unmerged for six months due to lack of a unit test. This branch does contain a test displaying that setState is called by the timeout only once.

Original PR description by @mstanaland:

Clears the setTimeout assigned to this.setCalendarInfoWidthTimeout before a new setTimeout is assigned/reassigned to prevent a possible memory leak.
Fixes issue #1323

@mackinleysmith
Copy link
Author

mackinleysmith commented Jul 14, 2020

Looks like the failures in Travis CI are not related to the code changed by this PR. @ljharb is this normal for this project?

EDIT: it looks like I was wrong, one of the failures is from my new test.
https://travis-ci.org/github/airbnb/react-dates/jobs/708032208#L1363

The error I'm getting (only in CI, it works for me locally) is can't redefine non-configurable property "window", which I got by calling this from mocha-wrap:

wrap().withGlobal('window', () => ({ getComputedStyle: sinon.stub().returns({}) }))

@ljharb it would be great to get your input on how to properly do this. I'm feeling a little stumped as to how to write a good test for this situation.

@ljharb
Copy link
Member

ljharb commented Jul 14, 2020

In the future, please do not create a second, duplicate PR - post a link to your branch and I can update the original PR. Now both of these need to stay open and kept in sync for the duration.

@ljharb
Copy link
Member

ljharb commented Jul 14, 2020

If that's the only failure, it's something I can try to look into in the next week or so.

@mackinleysmith
Copy link
Author

Okay sure, would you like me to close this PR?

@ljharb
Copy link
Member

ljharb commented Aug 3, 2020

@smit1625 no, please don't close it :-) i'll need to keep it in sync with #1736 until both are landed.

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.

None yet

2 participants