[TimerMixin] Remove TimerMixin from ListView

[exced]

Related to #21485

• Remove TimerMixin from ListView

Test Plan:

• All flow tests succeed.
• RNTester: iOS (this change should only affect iOS because calculateChildFrames is iOS only)
  Show perf monitor, show ListView* screen, start scrolling. UI frame Rate is used at the beginning. When scrolling there is no drop in FPS rate.

TODO: I think a load test would be more relevant:

• Update props multiple times and scroll

Release Notes:

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

[elicwhite]

Thanks for the PR! Please note the comment about the importance of test plans from the main issue. I don't think relying on Flow for this is sufficient. We've seen some surprising errors sneak in after having made these changes when they weren't tested by running the code.

When submitting a PR removing an instance of TimerMixin, it is important to include a good test plan. This test plan helps reviewers understand the work you did to verify that your changes are correct and complete. Since this changes the runtime logic of these components, it is important that these changes are tested in an app. For most of these components, you should be able to validate your change via RNTester. If there is nothing that exists as-is in RNTester which gives you confidence in your change, adding something to RNTester would be greatly appreciated. :)

[RSNara]

That was quick! Thanks for the PR :)

It looks mostly good, but I have a few concerns.

[RSNara]

_rafIds: Array<AnimationFrameID>

[RSNara]

Both componentDidMount and componentDidUpdate will write to this._rafId, which means that the rafId from componentDidUpdate will overwrite the rafId from componentDidMount. Suppose that both hooks are executed, and then the component is unmounted, all before the first requestAnimationFrame hook is executed. Then, with the current setup, we'll end up executing this._measureAndUpdateScrollProps(); once.

To get around this, we could add the following function to ListView:

requestAnimationFrame(fn: () => void): void {
  const rafId = requestAnimationFrame(() => {
    this._rafIds.splice(this._rafIds.indexOf(rafId));
    fn();
  })
  this._rafIds.push(rafId);
},

[exced]

You're right, I'll update it to have multiple references and come up with a test plan

[RSNara]

Looks good! 🧐

[facebook-github-bot]

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

[react-native-bot]

@exced merged commit 8ceb158 into facebook:master.