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

[TimerMixin] rm TimerMixin from TimersTest #21772

Closed
wants to merge 3 commits into from

Conversation

exced
Copy link
Contributor

@exced exced commented Oct 13, 2018

Related to #21485

Remove TimerMixin from TimersTest

Test Plan:

  • All flow tests succeed
  • yarn run test-android-instrumentation && CI integration tests ? (Did not run the command yet and not sure how to properly test it)

Release Notes:

[GENERAL] [ENHANCEMENT] [TimersTest.js] - rm TimerMixin

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 13, 2018
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.

IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
IntegrationTests/TimersTest.js Show resolved Hide resolved
Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

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

Looks really good!

But, I think there's a problem with setInterval. Please see the comments.


_setInterval: function(fn: () => void, ms?: number): IntervalID {
const intervalID = setInterval(() => {
this._intervalIDs.splice(this._intervalIDs.indexOf(intervalID));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bug? When the callback is first called, we'll clear the interval id from this_intervalIDs, which means that we'll never clear the interval in componentWillUnmount.

@exced
Copy link
Contributor Author

exced commented Oct 15, 2018

@RSNara You're write I shouldn't clear in _setInterval ! I fixed it
I also found out an issue with splice method !
I still don't know if you prefer using splice(index,1) or filter (what's the most efficient method ?)
(#21802 is a fix for the same issue)

@RSNara
Copy link
Contributor

RSNara commented Oct 15, 2018

@RSNara You're write I shouldn't clear in _setInterval ! I fixed it
I also found out an issue with splice method !
I still don't know if you prefer using splice(index,1) or filter (what's the most efficient method ?)
(#21802 has the same issue)

Nice catch on the Array.prototype.splice. We should have been more careful initially. 🤔

I think your call about using Array.prototype.filter is correct. It's less confusing than Array.prototype.splice. In the worst case, both have linear time complexity, so I'm not too worried about performance.

Alternatively, we could use Set instead of Array, since it'll be even more efficient, and conceptually, it also makes the most sense to use. Your call.

Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

RSNara has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@RSNara
Copy link
Contributor

RSNara commented Oct 18, 2018

@exced, I just noticed that #21748 does the exact same thing. I modified it to take the approach you took in this PR. Since that's already landing internally, I'll close this PR. Thanks for this PR! (And sorry that I didn't notice the conflict earlier 😢).

@RSNara RSNara closed this Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants